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

pprint_value_unit func for printing value and unit #4007

Merged
merged 1 commit into from Jan 14, 2020

Conversation

@DancingQuanta
Copy link
Contributor

DancingQuanta commented Sep 28, 2019

The printing of dimension value with unit is not consistent throughout,
eg missing space between unit and value. This is due to lack of
consistency in logic within construction of the value with unit string.
This PR adds a new function pprint_value_unit to Dimension based on
pprint_value_string that prints just a value and unit, if exists,
with a space between them.
This function replaces the pprint_value along with checks for unit.
This concentrates the logic within one place and this will improve
maintainability

@DancingQuanta DancingQuanta force-pushed the DancingQuanta:pprint branch from 681a666 to 9eb6e70 Oct 5, 2019
@DancingQuanta DancingQuanta force-pushed the DancingQuanta:pprint branch from 9eb6e70 to 3c15777 Nov 9, 2019
@philippjfr

This comment has been minimized.

Copy link
Member

philippjfr commented Nov 14, 2019

Sorry I left this sit for so long (I thought I'd left a comment but it seems I never hit submit). Instead of a new method I think I'd prefer an argument to the existing method.

@DancingQuanta DancingQuanta force-pushed the DancingQuanta:pprint branch 2 times, most recently from 6b9f30d to e9bd257 Nov 18, 2019
@@ -26,7 +26,9 @@ def test_dimension_clone(self):
def test_dimension_pprint(self):
dim = Dimension('Test dimension', cyclic=True, type=float, unit='Twilight zones')
self.assertEqual(dim.pprint_value_string(3.23451), 'Test dimension: 3.2345 Twilight zones')
self.assertEqual(dim.pprint_value_string(4.23441), 'Test dimension: 4.2344 Twilight zones')
self.assertEqual(dim.pprint_value_string(4.23441), 'Test dimension: 4.2344 Twilight zones')
self.assertEqual(dim.pprint_valueg(3.23451, print_unit=True), '3.2345 Twilight zones')

This comment has been minimized.

Copy link
@philippjfr

philippjfr Jan 2, 2020

Member
Suggested change
self.assertEqual(dim.pprint_valueg(3.23451, print_unit=True), '3.2345 Twilight zones')
self.assertEqual(dim.pprint_value(3.23451, print_unit=True), '3.2345 Twilight zones')
self.assertEqual(dim.pprint_value_string(4.23441), 'Test dimension: 4.2344 Twilight zones')
self.assertEqual(dim.pprint_value_string(4.23441), 'Test dimension: 4.2344 Twilight zones')
self.assertEqual(dim.pprint_valueg(3.23451, print_unit=True), '3.2345 Twilight zones')
self.assertEqual(dim.pprint_valueg(4.23441, print_unit=True), '4.2344 Twilight zones')

This comment has been minimized.

Copy link
@philippjfr

philippjfr Jan 2, 2020

Member
Suggested change
self.assertEqual(dim.pprint_valueg(4.23441, print_unit=True), '4.2344 Twilight zones')
self.assertEqual(dim.pprint_value(4.23441, print_unit=True), '4.2344 Twilight zones')
@philippjfr

This comment has been minimized.

Copy link
Member

philippjfr commented Jan 2, 2020

Two typos in the tests, otherwise I'm happy to merge.

@philippjfr

This comment has been minimized.

Copy link
Member

philippjfr commented Jan 6, 2020

@DancingQuanta Any chance you could apply my suggestions? Just need to click "Commit suggestion".

@DancingQuanta DancingQuanta force-pushed the DancingQuanta:pprint branch from e9bd257 to 013309e Jan 14, 2020
@DancingQuanta DancingQuanta force-pushed the DancingQuanta:pprint branch from 013309e to 2104c90 Jan 14, 2020
@DancingQuanta

This comment has been minimized.

Copy link
Contributor Author

DancingQuanta commented Jan 14, 2020

Hi @philippjfr, I have added your suggestions and rebased on current master.

@philippjfr

This comment has been minimized.

Copy link
Member

philippjfr commented Jan 14, 2020

Much appreciated!

@philippjfr philippjfr merged commit a511f22 into holoviz:master Jan 14, 2020
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.