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

Use ComparisonMode constants instead of magic numbers #1229

Merged
merged 3 commits into from
Jun 26, 2020

Conversation

mdickinson
Copy link
Member

Minor cleanup PR: replace uses of magic numbers for comparison modes with the corresponding ComparisonMode elements. (The numbers should eventually become just an implementation detail; we want to encourage all traits users to use the ComparisonMode constants instead.)

@mdickinson
Copy link
Member Author

Note: this PR will conflict with #1165; whichever one gets merged second (probably this one) will need to deal with the merge conflicts.

@mdickinson
Copy link
Member Author

Hmm. I somehow lost some of the changes when resolving conflicts. Will fix.

@mdickinson
Copy link
Member Author

Ah no; I see - there were new cases introduced in #1208.

@mdickinson
Copy link
Member Author

Updated after a master merge. Though now that #1165 is in, I'm wondering whether some of those comparison_mode=ComparisonMode.identity annotations can be removed. @kitchoi ?

@@ -417,12 +422,12 @@ class Potato(HasTraits):

class PotatoBag(HasTraits):

potatos = List(Instance(Potato), comparison_mode=1)
potatos = List(Instance(Potato), comparison_mode=ComparisonMode.identity)
Copy link
Member Author

Choose a reason for hiding this comment

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

I really want to change this to potatoes, but that's out of scope for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops. 😂

@kitchoi
Copy link
Contributor

kitchoi commented Jun 26, 2020

Race conditions from a number of PRs. Yes I agree the many ComparisonMode.identity aka 1, can be removed.

@mdickinson
Copy link
Member Author

@kitchoi Okay to merge this and look at removing redundant comparison_mode=... metadata later?

Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

Yep! LGTM

@mdickinson mdickinson merged commit 74c8206 into master Jun 26, 2020
@mdickinson mdickinson deleted the cln/comparison-mode branch June 26, 2020 17:01
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.

2 participants