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

fixed bug with rstrip #382

Merged
merged 5 commits into from
Jun 20, 2019
Merged

fixed bug with rstrip #382

merged 5 commits into from
Jun 20, 2019

Conversation

adnanhemani
Copy link
Member

[No] Wrote test for feature
[X] Added changes in the Changelog section in README.md
[X] Bumped version number (delete if unneeded)

Changes proposed:
Fixed the bug as described in #338. Was caused as a side-effect of using rstrip which will truncate a set of characters from the back of a string rather than a substring - causing a label like "proportion" to get truncated to "proporti" because "i" was the first letter not in the set of characters " ,c,o,u,n,t".

@coveralls
Copy link

coveralls commented Jun 18, 2019

Coverage Status

Coverage remained the same at 76.005% when pulling 9e5cab1 on hist_x_label_truncated into 61a301a on master.

@adnanhemani
Copy link
Member Author

Again, I'm unsure as to how to test this properly.

Untitled.ipynb Outdated Show resolved Hide resolved
datascience/tables.py Outdated Show resolved Hide resolved
datascience/tables.py Outdated Show resolved Hide resolved
@davidwagner
Copy link
Member

I'm unsure as to how to test this properly.

If it doesn't mess up any of the histograms in tests/Charts.ipynb, and fixes the problem in #337, I'd say that we have probably tested it sufficiently.

In the long run, we might be able to automate some of the testing for histograms using https://matplotlib.org/devel/testing.html#writing-an-image-comparison-test.

Copy link
Member

@davidwagner davidwagner left a comment

Choose a reason for hiding this comment

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

The core fix looks good, but let's clean up the extra stuff that also got added with it in this pull request.

@adnanhemani
Copy link
Member Author

Made all the fixes - thanks for the assist!

@davidwagner davidwagner self-requested a review June 20, 2019 04:45
Copy link
Member

@davidwagner davidwagner left a comment

Choose a reason for hiding this comment

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

Looks great!

@davidwagner davidwagner merged commit b65bafa into master Jun 20, 2019
@adnanhemani adnanhemani deleted the hist_x_label_truncated branch June 20, 2019 05:56
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.

None yet

3 participants