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

Conversation

Projects
None yet
2 participants
@jlstevens
Member

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 jlstevens self-assigned this Mar 29, 2017

@jlstevens jlstevens added API WIP labels Mar 29, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 29, 2017

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

@philippjfr philippjfr removed the WIP label Mar 29, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Mar 29, 2017

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))

This comment has been minimized.

@philippjfr

philippjfr Mar 30, 2017

Member

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.

This comment has been minimized.

@jlstevens

jlstevens Mar 30, 2017

Member

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?

This comment has been minimized.

@philippjfr

philippjfr Mar 30, 2017

Member

Yes, that seems right.

This comment has been minimized.

@jlstevens
@philippjfr

This comment has been minimized.

Member

philippjfr commented Mar 30, 2017

Looks good now, will merge when tests pass.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 30, 2017

Tests now passing except for one transient error.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Mar 30, 2017

Will merge, since the PR build has passed.

@philippjfr philippjfr merged commit 6e8d2ec into master Mar 30, 2017

3 of 4 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.004%) to 78.082%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the clear_subscribers branch Apr 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment