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

Relax immutability in NamedTraitObserver for consistency #1082

Merged
merged 1 commit into from
May 13, 2020

Conversation

kitchoi
Copy link
Contributor

@kitchoi kitchoi commented May 13, 2020

ListItemObserver does not prevent its attributes to be mutated (see #1071). For consistency, this PR relaxes the protection on NamedTraitObserver too.

These attributes should not be mutated despite this change.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • Update User manual (docs/source/traits_user_manual)
  • Update type annotation hints in traits-stubs

@kitchoi
Copy link
Contributor Author

kitchoi commented May 13, 2020

FTR, I'd prefer keeping this protection, but consistency is more important.

@mdickinson
Copy link
Member

I'd prefer keeping this protection

Okay; can you explain why? Maybe I'm missing something. Do you consider this particular bit of code in special need of protection? Do you think we should have this level of protection everywhere in the codebase, or is it just here that it's particularly important?

@kitchoi
Copy link
Contributor Author

kitchoi commented May 13, 2020

I am wiling to bring about this change, and I rate this less important than consistency. I just mean not to take credit for the change, that is all. I am sorry for the confusion.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM

@kitchoi kitchoi merged commit db76bc8 into master May 13, 2020
@kitchoi kitchoi deleted the relax-immutable branch May 13, 2020 16:36
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.

None yet

2 participants