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

Simplified how Event subscribers are handled #1235

Merged
merged 17 commits into from
Mar 30, 2017
Merged

Conversation

jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Mar 29, 2017

This PR offers clear and add_subscriber methods to Event and does away with _hidden_subscribers. The subscribers attribute is now in fact a read-only property which means the correct way to add subscribers is with the add_subscribers method.

Outstanding items to address:

  • Clarify whether DynamicMap.event and Stream.update methods support names before or after renaming.
  • Add more unit tests
  • Add docstrings

@jlstevens
Copy link
Contributor Author

@philippjfr I think it is ready for review - I'm expecting the tests to pass.

@philippjfr
Copy link
Member

Addresses #1168

stream.update(**dict({k:kwargs[k] for k in overlap}, trigger=False))
updated_streams.append(stream)
rkwargs = util.rename_stream_kwargs(stream, kwargs, reverse=True)
stream.update(**dict(rkwargs, trigger=False))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm not following the logic correctly in the utilities but won't this try to send the supplied kwargs to all the streams even though they may not apply? That's what the overlap bit was for I assume.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that has helped me realize what we need:

  • An initial loop over set(kwargs.keys()) - set(all_streams) to raise an exception for anything in that difference set: these are kwargs that cannot be applicable to any stream. Previously invalid kwargs were silently ignored and I think this is bad behavior unless you have reasons to say otherwise.

  • util.rename_stream_kwargs(stream, {k:v for k,v in kwargs.items() if k in set(stream.contents.keys())}, reverse=True) to filter the kwargs to the applicable ones.

Do you agree that this is the right approach?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that seems right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 79b7de1

@philippjfr
Copy link
Member

Looks good now, will merge when tests pass.

@jlstevens
Copy link
Contributor Author

Tests now passing except for one transient error.

@philippjfr
Copy link
Member

Will merge, since the PR build has passed.

@philippjfr philippjfr merged commit 6e8d2ec into master Mar 30, 2017
@philippjfr philippjfr deleted the clear_subscribers branch April 11, 2017 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants