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

add rightonly jsondiff syntax #60

Merged
merged 5 commits into from
Jun 20, 2023
Merged

add rightonly jsondiff syntax #60

merged 5 commits into from
Jun 20, 2023

Conversation

ramwin
Copy link
Contributor

@ramwin ramwin commented Apr 26, 2023

I some senario, we often compare old value and new value and check if the newer value is reasonabel. So the rightonly syntax will compute the dicfference and keep the later one compared to CompactJsonDiffSyntax

Copy link
Member

@fzumstein fzumstein left a comment

Choose a reason for hiding this comment

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

didn't know you have to submit review for comments to become visible. anyhow, the hash issue has now been fixed.

@@ -12,6 +12,10 @@ def __str__(self):
def __eq__(self, other):
return self.label == other.label

def __hash__(self):
Copy link
Member

Choose a reason for hiding this comment

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

Given how eq works, I think this should be

def __hash__(self):
    return hash(self.label)

Copy link
Member

Choose a reason for hiding this comment

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

this was fixed in #61

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -8,6 +9,15 @@
from hypothesis import given, settings, strategies


logging.basicConfig(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like debugging code. Did you intend to leave this in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's convenient to add some log during test.

@corytodd
Copy link
Collaborator

corytodd commented Jun 4, 2023

IntuitiveSyntax is interesting! If you're still working on this it would be really cool to have as an option. Maybe a followup commit to strip this PR down to just that feature?

@corytodd
Copy link
Collaborator

corytodd commented Jun 4, 2023

After testing this some more I'm questioning the name intuitive. The names of the other syntax objectively describe their output so I worry that others might not find this output intuitive for their data.

Some ideas:

  • unified: output has full context which reminds me of git diff --unified
  • compact-context: compact is extended to include more context

@ramwin
Copy link
Contributor Author

ramwin commented Jun 15, 2023

In my senario, I only what to know what field has changed. And check if the newest value is reasonable.
How about using 'rightonly', 'keepright' as the syntax name?

@corytodd
Copy link
Collaborator

Those sound reasonable to me! +1 for rightonly.

@corytodd
Copy link
Collaborator

Can you edit the title to reflect the latest changes and add a PR description explaining your changes? That will make it easier to understand months from now.

@ramwin ramwin changed the title add intuitive jsondiff syntax && fix unhashable type error add rightonly jsondiff syntax Jun 19, 2023
@ramwin
Copy link
Contributor Author

ramwin commented Jun 19, 2023

@corytodd I have updated the pull request title and added a PR description. Thank you for your advice.

Copy link
Collaborator

@corytodd corytodd left a comment

Choose a reason for hiding this comment

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

LGTM

@fzumstein fzumstein merged commit c0aab2a into xlwings:master Jun 20, 2023
@fzumstein
Copy link
Member

thanks @ramwin and @corytodd !

@bew
Copy link

bew commented Aug 22, 2023

Hello, do you intend to make a release soon with this PR?

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.

4 participants