Skip to content
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

Update sync_from and sync_to to return the diff and the status of the sync #77

Closed
dgarros opened this issue Oct 28, 2021 · 7 comments · Fixed by #188
Closed

Update sync_from and sync_to to return the diff and the status of the sync #77

dgarros opened this issue Oct 28, 2021 · 7 comments · Fixed by #188
Assignees

Comments

@dgarros
Copy link
Contributor

dgarros commented Oct 28, 2021

Environment

  • DiffSync version: 1.3.0

Proposed Functionality

Currently the functions sync_to/from are not returning anything, whether the sync was completed or not.
It would be useful to return at least the status of the sync and eventually the diff that was generated by the function.

Use Case

Developer experience

@Kircheneer
Copy link
Collaborator

This can be done here.

@dakonr
Copy link
Contributor

dakonr commented Nov 15, 2022

This can be done here.

Is that issue a task wich can be handled by a new contributor?

@Kircheneer
Copy link
Collaborator

Yes absolutely @dakonr

In fact I had the hacktoberfest label on it until a couple of days ago. Feel free to submit a draft PR if you're unsure about something and you will get some feedback. Thanks!

@dakonr
Copy link
Contributor

dakonr commented Nov 16, 2022

ok @Kircheneer I will try my best, you can assign that issue to me (I can't do that by myself)

First of all I need to understand the requirement and the intended behaviour.

  • The easy way could be to retrun always true at the end of the function. The code looks like, that on some stop points an error will raise.
  • An other solution is to return the diff. That can be used to check the sync and will interpret to true BUT also as false if there is no diff.
  • The third solution can return a tuple with state of the sync as first value and the diff as second value.

I couldn't find any clear design principles about that. Wich one will rise the DX @dgarros ?

@Kircheneer
Copy link
Collaborator

I just went through the code and with the way the API is currently structured I am not sure how we can get useful information about a sync status. Like you said, for example here we raise an exception rather then defining some error message to be returned. Since this is a common Python pattern I would prefer that we stick with it rather then returning the result information from the methods. Therefore I vouch for just returning the diff with the successful return of the function signaling execution success. Any differing opinion @glennmatthews?

@glennmatthews
Copy link
Collaborator

I think that makes sense, yeah.

@dakonr
Copy link
Contributor

dakonr commented Nov 18, 2022

I agree with you both, the diff will be the best solution and will return more implicit information to the calling procedure.
So, I will start with an implementation of that solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants