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

tuple generator #57

Merged
merged 9 commits into from Mar 3, 2017
Merged

tuple generator #57

merged 9 commits into from Mar 3, 2017

Conversation

bmcfee
Copy link
Collaborator

@bmcfee bmcfee commented Mar 2, 2017

This implements #56 , and provides a tuple generator interface for all streamer objects.

To-do:

  • core implementation
  • core unit test for streamer
  • integration tests for all derived classes
  • cyclic tuple support

@bmcfee bmcfee added this to the 1.0 milestone Mar 2, 2017
@bmcfee bmcfee self-assigned this Mar 2, 2017
@bmcfee bmcfee requested a review from cjacoby March 2, 2017 21:29
@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 2, 2017

I added cycle support as an optional kwarg to tuples. Maybe this would be better as a separate method, but it seems kinda redundant. What do yall think?

@bmcfee bmcfee changed the title [WIP] tuple generator tuple generator Mar 2, 2017
@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 2, 2017

I think this one's good to go -- tuple testing is integrated across all subclasses.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 2, 2017

.. getting a strange heisenbug on zmq streamer though. Possibly timeout related. @cjacoby @ejhumphrey want to take a look?

@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 3, 2017

Hrm. Heisenbug averted? This might take a more careful look.

For now though, I think ZMQ issues are safely independent of this PR, so it should be ready for review (and merge, I think).

Copy link
Collaborator

@cjacoby cjacoby left a comment

Choose a reason for hiding this comment

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

LGTM. I have done one pass through the changes. Could try to go deeper.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 3, 2017

Could try to go deeper.

Probably not worth it -- the changes are relatively superficial.

If there's anything to think about, maybe it's whether this is really the interface we want. I think it is, but I'm open to suggestions.

@cjacoby
Copy link
Collaborator

cjacoby commented Mar 3, 2017 via email

@bmcfee bmcfee merged commit f1a6512 into master Mar 3, 2017
@bmcfee bmcfee deleted the tuple-generator branch March 3, 2017 01:52
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.

None yet

2 participants