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

Add missing property index_range to 1D plots #491

Merged
merged 8 commits into from
Nov 19, 2019

Conversation

nicolasap-dm
Copy link
Contributor

@nicolasap-dm nicolasap-dm commented Nov 18, 2019

Adds a index_range property, synonym of index_mapper.range, to Base1DPlot. This corresponds to a similarly defined property in Base2DPlot, and Plot._handle_range_changed() expects this attribute to exist in order to work properly.

+ new assertions for regression testing.

Closes #490

@nicolasap-dm
Copy link
Contributor Author

All python 3.6 CI builds are failing at the setup stage with

 OkonomiyakiError("Unsupported PKG-INFO metadata format: u'2.1'",)

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

A couple of minor nits.

Also there isn't a direct test of setting the index_range trait, only a second-order test of changing the index range of a Plot. It probably makes sense to add a setting test to the test case, and possibly a test that when you change the index_mapper that the index_range fires a trait change and is updated as expected (whether you do this depends on how much you trust the traits internals :) ).

@nicolasap-dm
Copy link
Contributor Author

I've addressed the comments.

test that when you change the index_mapper that the index_range fires a trait change

I didn't have a module-level listener on index_range so I hooked one up to test, I hope that's canon

I made a drive-by style fix: all test functions were called "*scatter_" even for non-scatter plot modules. Since it may be off-topic, I committed it separately (cdd3cc7) to be able to revert it.

@nicolasap-dm
Copy link
Contributor Author

Thanks, I didn't know about the mixin. I've made the changes.

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

LGTM

@nicolasap-dm nicolasap-dm merged commit d5f9496 into master Nov 19, 2019
@nicolasap-dm nicolasap-dm deleted the fix/missing-baseplot1d-index-range branch November 19, 2019 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base1DPlot doesn't have a index_range trait, doesn't listen to changes upstream
2 participants