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

Fix steaming for DataFrame-based ColumnDataSource #6738

Merged
merged 3 commits into from Aug 20, 2017

Conversation

p-himik
Copy link
Contributor

@p-himik p-himik commented Aug 7, 2017

Fixes #6667

@bryevdv
Copy link
Member

bryevdv commented Aug 16, 2017

@p-himik just a heads up, for this to go into 0.12.7 it will need t be wrapped up by Monday or so

@bryevdv
Copy link
Member

bryevdv commented Aug 16, 2017

FYI the changed code in the merge conflict is to deal with the multi-index case, it makes a CDS key by joining the sub-index col names with _. The new code may be OK as-is for the rest of your PR, but you should take look.

@p-himik
Copy link
Contributor Author

p-himik commented Aug 17, 2017

@bryevdv The tests contain a fair amount of repetitions - not only mine but many tests in test_sources.py. What do you think about rewriting this file in pytest? I've been using it quite intensively lately, and I think that file can be rewritten in a way that offers a better coverage with a smaller code size.

@bryevdv
Copy link
Member

bryevdv commented Aug 17, 2017

@p-himik we try to use pytest for all new tests, and I would not be opposed to converting old tests as feasible. I will say even though we use pytest for new tests we don't have alot of experience e.g. with parameterized tests so some good examples of doing that or other best practices would be great

@p-himik p-himik requested a review from bryevdv August 20, 2017 04:00

ds.data._stream = stream_wrapper

ds._stream(pd.Series([11, 21, 31], index=list('abc')), 7)
Copy link
Member

Choose a reason for hiding this comment

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

OH this is interesting, I didn't realize Series could be used this way.

@bryevdv
Copy link
Member

bryevdv commented Aug 20, 2017

@p-himik Thanks the tests look good. I agree that there is a fair amount of boilerplate, but I'm willing to defer cleanup up on that until the entire module gets converted to use pytest at some point.

Thanks!!

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