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

Migration to identity comparison mode for container traits #1122

Closed
kitchoi opened this issue May 20, 2020 · 6 comments
Closed

Migration to identity comparison mode for container traits #1122

kitchoi opened this issue May 20, 2020 · 6 comments
Milestone

Comments

@kitchoi
Copy link
Contributor

kitchoi commented May 20, 2020

#1117 adds a warning if container mutations are observed and the trait comparison mode is set to equality (the default). To fix the warning, one needs to manually set the comparison mode to identity, which may be tedious.

We need a way to migrate code to minimize the impact of these warnings.

Ideas:

  • Provide an easy way for projects to set comparison mode to identity, preferably one that can be done with find-all-and-replace, such as an alias
  • Eventually make comparison_mode=ComparisonMode.identity the default for List/Dict/Set, possibly with advanced warnings through a number of releases

Excerpt from #1117 (review):

One possible plan is to eventually make comparison_mode=ComparisonMode.identity the default for these trait types; we'd have to do that after most code had transitioned to using observe, and with suitable warnings (probably over several releases).
Another possibility (orthogonal to the first) would be to provide convenience aliases: e.g., IList(...) = List(..., comparison_mode=ComparisonMode.identity). (Thinking "I" for "Identity", but we'd have to figure out the right name.)

Excerpt from #1117 (comment)

In some future release, pull comparison_mode out of the keyword arguments of List.init (and friends), with the default None in the signature. If it is None, use ComparisonMode.equality for backward compatibility but put up a deprecation warning. The deprecation warnings are going to be very noisy if code has not done the switch early on.
...
Note there is also a scenario where Union is used with a container type as its inner trait, and the identity comparison mode needs to be defined on the Union, not on the container type (see test in this PR).

@mdickinson
Copy link
Member

Putting this into 6.2.0 so that it's on our radar. We likely can't complete this in 6.2.0, but we can aim to have a plan by the 6.2.0 release.

@kitchoi
Copy link
Contributor Author

kitchoi commented Jun 26, 2020

Did #1165 make this issue obsolete such that this issue can be closed? Or does the proposal still stand in its own right?

@kitchoi
Copy link
Contributor Author

kitchoi commented Jun 26, 2020

Since I opened the issue, I should say what I think: I think this can be closed.

@mdickinson
Copy link
Member

Yes, I think we can call this obsolete.

@kitchoi
Copy link
Contributor Author

kitchoi commented Jun 26, 2020

Excellent. Racing to the close button...

@kitchoi kitchoi closed this as completed Jun 26, 2020
@mdickinson
Copy link
Member

Darn. Too slow. Again.

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

No branches or pull requests

2 participants