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 operation modes supported by Dynamicmap #1238

Merged
merged 14 commits into from
Mar 31, 2017

Conversation

jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Mar 30, 2017

This PR greatly simplified DynamicMap, removing all concept of 'modes' except for 'sampled' mode: 'open', 'bounded', 'generator' and 'counter' mode are all gone (implicitly DynamicMaps are now always 'bounded').

In addition, keys cannot be returned by the callback.

Action items:

  • Slash all the code relating to the DynamicMap modes

Edit:

The following items will be assigned to a follow up PR after this one is merged.

  • Reintroduce validation which will now relate to streams (especially dimensioned streams)
  • Add some new tests
  • Update the Tutorials

I might recommend leaving the last point (updating the tutorials) for another PR - the earlier this is merged, the better tested it will be before release and the bigger chance we can hear any objections from our users. We don't think anyone has ever really other used anything other than 'bounded' mode with callables (possibly with streams) but we can't be certain.

@jlstevens
Copy link
Contributor Author

@philippjfr I now think this PR should be about cleaning things up only.

Once we have removed everything we want to, I think we should merge and I'll follow up with a PR to add things back (updating docs, adding tests, adding more validation etc).

I will delete the bits in the DynamicMap tutorial that are out of date (it might no longer be a coherent story but that is what I'll fix in the next PR). Sound reasonable?

@jlstevens
Copy link
Contributor Author

I've now removed the sections of the DynamicMap tutorial that no longer apply - everything still in the notebook works as before. In the next PR I'll rewrite this tutorial to try and cover the same sort of examples but using streams. We might also want to decide on how to split between a DynamicMap tutorial and a streams tutorial.

@philippjfr I think this PR is ready for review - is there anything else that can be cleaned up and removed before I work on reintroducing things?

@jbednar
Copy link
Member

jbednar commented Mar 31, 2017

I always like to see lots of code disappear. 700 lines gone!

@philippjfr
Copy link
Member

Looks great to me!

@jlstevens
Copy link
Contributor Author

jlstevens commented Mar 31, 2017

Feel free to click 'Merge' then! :-)

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

Successfully merging this pull request may close these issues.

3 participants