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

Implement SetItemObserver for observing mutations on a set #1075

Merged
merged 18 commits into from
May 15, 2020

Conversation

kitchoi
Copy link
Contributor

@kitchoi kitchoi commented May 11, 2020

This PR implements item 10 of #977 for observing mutations on a set (strictly speaking, TraitSet).

  • Add SetChangeEvent to represent mutations on a set
  • Implement IObserver interface to
    • Return the TraitSet object for adding/removing notifiers
    • Yield the content of the set for the next observer(s) in the observer graph.
      - Define the notifier such that SetChangeEvent will be created for the user's change handler.
      - Implement logic for maintaining downstream observers
  • Implement the interface of IObservable for TraitSet. With that, any subclass of TraitSet can be used with SetItemObserver.

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

@codecov-io
Copy link

codecov-io commented May 11, 2020

Codecov Report

Merging #1075 into master will decrease coverage by 2.36%.
The diff coverage is 66.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1075      +/-   ##
==========================================
- Coverage   76.15%   73.79%   -2.37%     
==========================================
  Files          54       74      +20     
  Lines        6493     8353    +1860     
  Branches     1263     1592     +329     
==========================================
+ Hits         4945     6164    +1219     
- Misses       1205     1809     +604     
- Partials      343      380      +37     
Impacted Files Coverage Δ
traits/api.py 100.00% <ø> (+9.67%) ⬆️
traits/base_trait_handler.py 61.76% <ø> (ø)
traits/ctrait.py 71.07% <ø> (ø)
traits/has_traits.py 72.40% <ø> (-0.37%) ⬇️
traits/observers/_i_notifier.py 0.00% <0.00%> (ø)
traits/observers/events.py 0.00% <0.00%> (ø)
traits/traits.py 75.10% <ø> (-2.45%) ⬇️
traits/util/resource.py 15.25% <ø> (ø)
traits/observers/_generated_parser.py 51.96% <51.96%> (ø)
traits/trait_types.py 72.15% <80.00%> (-0.31%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c8922c...e57bad0. Read the comment docs.

@mdickinson mdickinson self-requested a review May 14, 2020 12:11
# then
event, = events
self.assertEqual(event.added, set([4]))
self.assertEqual(event.removed, set([]))
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: set() is the usual way to spell the empty set. (Could also use {4} in the line above.)



def create_observer(**kwargs):
""" Convenient function for creating SetItemObserver with default values.
Copy link
Member

Choose a reason for hiding this comment

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

typo: "Convenient" -> "Convenience"

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. Should we wait until #1071 is merged before merging this one, just in case there are last minute changes to #1071 that require corresponding for-consistency changes in this PR?

@kitchoi
Copy link
Contributor Author

kitchoi commented May 14, 2020

Should we wait until #1071 is merged before merging this one, just in case there are last minute changes to #1071 that require corresponding for-consistency changes in this PR?

Yes! Will do that.

@mdickinson
Copy link
Member

One typo to fix and merge conflicts to sort out; other than that, I think this is ready to merge.

@kitchoi kitchoi merged commit 3ccec07 into master May 15, 2020
@kitchoi kitchoi deleted the 977-set-item-observer branch May 15, 2020 08:59
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.

3 participants