Skip to content

Add add_or_update to DiffSync class. #70

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
Nov 12, 2021
Merged

Add add_or_update to DiffSync class. #70

merged 19 commits into from
Nov 12, 2021

Conversation

FragmentedPacket
Copy link
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

Copy link
Collaborator

@glennmatthews glennmatthews left a comment

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
Contributor Author

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

glennmatthews
glennmatthews previously approved these changes Nov 8, 2021
Copy link
Collaborator

@glennmatthews glennmatthews left a comment

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
Contributor Author

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

Copy link
Contributor

@dgarros dgarros left a comment

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
Contributor Author

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

@FragmentedPacket
Copy link
Contributor Author

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

Copy link
Contributor

@dgarros dgarros left a comment

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
Copy link
Contributor

dgarros commented Nov 12, 2021

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

I think it does

@dgarros
Copy link
Contributor

dgarros commented Nov 12, 2021

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