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 new Formatting classes and corresponding tests #186

Merged
merged 12 commits into from
Apr 6, 2018

Conversation

ben-cunningham
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Apr 4, 2018

Coverage Status

Coverage decreased (-0.03%) to 93.149% when pulling e0e2e08 on ben-cunningham:milestone7_pr into bec7f78 on sandialabs:master.

@tshead2
Copy link
Member

tshead2 commented Apr 4, 2018

@ben-cunningham : Really appreciate this request, but I see one issue - the new unit and currency formatters are changing the API by returning four values instead of three. This would break rendering for tables (where formatters are used) ... are you using formatters in some other context? Regardless, if you combine the suffix and units in the UnitFormatter, and combine the currency code and prefix in the CurrencyFormatter, I'll merge everything.

Cheers,
Tim

@ben-cunningham
Copy link
Contributor Author

@tshead2 Thanks! I've hopefully fixed up the issues, the Formatters should just be returning three values now. Is there a way to fix coveralls passing?

@tshead2
Copy link
Member

tshead2 commented Apr 5, 2018

@ben-cunningham BTW, I appreciate you taking the time to add test coverage … take a look at https://coveralls.io/builds/16346816/source?filename=toyplot%2Fformat.py to see the lines (in red) that aren’t covered by the new tests. I generally put # pragma: no cover on lines that throw exceptions, but the others should get tests (I see that at least one omission is mine).

Cheers,
Tim

@ben-cunningham ben-cunningham force-pushed the milestone7_pr branch 2 times, most recently from 2a772ec to f53b1a7 Compare April 5, 2018 03:35
@ben-cunningham
Copy link
Contributor Author

@tshead2 Hey, I've added tests for strings and numpy.nan but according to coveralls the lines aren't getting hit for some reason...

@tshead2 tshead2 merged commit 1c0a5cd into sandialabs:master Apr 6, 2018
@tshead2
Copy link
Member

tshead2 commented Apr 6, 2018

This looks close enough to me, I'll merge it and look into the tests.

Many thanks!
Tim

@tshead
Copy link
Contributor

tshead commented Apr 23, 2018

@ben-cunningham - just a heads up that I did some cleanup of the formatter code to reduce repetition. You might want to take a look and confirm that I didn’t break anything for you. I also updated the regression tests for 100% coverage - you might be interested to see how I was able to dramatically reduce the number of steps.

Cheers,
Tim

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

5 participants