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

Projects
None yet
3 participants
@jlstevens
Member

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 jlstevens added API WIP labels Mar 30, 2017

@jlstevens jlstevens added this to the v1.7.0 milestone Mar 30, 2017

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 31, 2017

@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

This comment has been minimized.

Member

jlstevens commented Mar 31, 2017

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

This comment has been minimized.

Member

jbednar commented Mar 31, 2017

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

@philippjfr

This comment has been minimized.

Member

philippjfr commented Mar 31, 2017

Looks great to me!

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 31, 2017

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

@philippjfr philippjfr merged commit 92ac5b5 into master Mar 31, 2017

4 checks passed

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

@philippjfr philippjfr deleted the dynamicmap_refactor branch Apr 11, 2017

@jlstevens jlstevens referenced this pull request Apr 17, 2017

Merged

Improve DynamicMap usability and deprecate sampled mode #1305

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