Skip to content

Add add_or_update to DiffSync class.#70

Merged
dgarros merged 19 commits into
networktocode:mainfrom
FragmentedPacket:50-get-or-create
Nov 12, 2021
Merged

Add add_or_update to DiffSync class.#70
dgarros merged 19 commits into
networktocode:mainfrom
FragmentedPacket:50-get-or-create

Conversation

@FragmentedPacket

Copy link
Copy Markdown
Contributor

Fixes #50

  • Adds add_or_update to DiffSync class that requires a DiffSyncModel to be passed in and will attempt to add or update an existing object
  • Convert one of the examples over to use the new method
  • Add test to validate proper functionality

@glennmatthews glennmatthews left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

While this looks like an interesting feature I'm not sure it fully addresses #50. I think I'd like to see functionality for all of the following to be able to consider #50 as "done":

  • Allow add()ing the same identical object repeatedly without it being considered an error condition. (No update() as part of this call, and in fact if the new object differs in any attrs from the previously added object, this should still raise ObjectAlreadyExists)
  • get_or_create functionality, similar to Django's, that either retrieves a previously stored item (whose ids and attrs match the provided values) or instantiates and add()s a new item, then returns it. Probably would be useful to follow Django's example and have this function return a tuple (record, bool_created)
  • update_or_create functionality to either find an existing object by ids and update with the provided attrs, or create a new object with the given ids and attrs, and in either case return the object. (Again, returning (record, bool_created) would be a good plan)

I'm also uncertain whether it's correct/desirable to call obj.update() as a part of any of the above APIs, versus directly just updating its attrs without making such a call - recall that obj.update() implies a change to be propagated/written to the underlying system or database, which should normally only happen as a result of a DiffSync.sync() call.

@FragmentedPacket

Copy link
Copy Markdown
Contributor Author

@glennmatthews Do you mind reviewing my latest commits to make sure I'm on the right path?

Comment thread diffsync/__init__.py Outdated
Comment thread diffsync/__init__.py Outdated
Comment thread diffsync/__init__.py Outdated
Comment thread diffsync/__init__.py Outdated
Comment thread examples/01-multiple-data-sources/backend_b.py Outdated
Comment thread tests/unit/test_diffsync.py Outdated
Comment thread diffsync/__init__.py Outdated
Comment thread tests/unit/test_diffsync.py Outdated
Comment thread diffsync/__init__.py Outdated
Comment thread diffsync/__init__.py Outdated
Comment thread diffsync/__init__.py Outdated
Comment thread examples/04-get-update-instantiate/backends.py Outdated
Comment thread examples/04-get-update-instantiate/backends.py
Comment thread tests/unit/test_diffsync.py Outdated
Comment thread tests/unit/test_diffsync.py Outdated
Comment thread tests/unit/test_diffsync.py
Comment thread tests/unit/test_diffsync.py Outdated
Comment thread diffsync/__init__.py
FragmentedPacket and others added 3 commits November 8, 2021 07:07
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
Co-authored-by: Glenn Matthews <glenn.matthews@networktocode.com>
glennmatthews
glennmatthews previously approved these changes Nov 8, 2021

@glennmatthews glennmatthews left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff!

@FragmentedPacket

Copy link
Copy Markdown
Contributor Author

@dgarros Not sure if you want to review it before it gets merged in.

@dgarros dgarros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great work @FragmentedPacket and thanks helping with this one

One last change for me before merging
please. can you add a README.md in the new example directory (04-XX) and add a link to this readme file in the documentation > diffsync/docs/source/examples/index.rst

@FragmentedPacket

Copy link
Copy Markdown
Contributor Author

@dgarros I completely forgot to even write any docs for this so thanks for catching that.

@FragmentedPacket

Copy link
Copy Markdown
Contributor Author

Does this also address #73? Just want to make sure once this is merged, we can close it.

@dgarros dgarros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @FragmentedPacket

@dgarros

dgarros commented Nov 12, 2021

Copy link
Copy Markdown
Contributor

Does this also address #73? Just want to make sure once this is merged, we can close it.

I think it does

@dgarros

dgarros commented Nov 12, 2021

Copy link
Copy Markdown
Contributor

I'll merge since Glenn already approved it

@dgarros dgarros merged commit b59a362 into networktocode:main Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Get or create function

3 participants