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

Formatting of decimal columns #7

Merged
merged 5 commits into from
May 31, 2017
Merged

Formatting of decimal columns #7

merged 5 commits into from
May 31, 2017

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented May 29, 2017

  1. Acceptance tests
    screenshot from 2017-05-29 22-43-17

  2. Refactoring, and grey out insignificant leading zeros
    screenshot from 2017-05-29 22-43-00

@hadley: Could you please review (and enable reviews)?

- add acceptance test
- `show_output_in_terminal()`
- import rlang
- simplify output for tests
- split into functions
- `style_num()` gains `subtle` argument
- also mark lhs and dot as insignificant if lhs is zero
@hadley
Copy link
Member

hadley commented May 30, 2017

BTW I think only collaborators can request reviews. I've added you since you're likely to want to contribute other stuff.


structure(
list(
neg = format_neg(neg),
Copy link
Member

Choose a reason for hiding this comment

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

We should probably call these format methods at print time, rather than creation. But that might imply a more significant refactoring than you want to do here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about storing the decomposition (neg, lhs, rhs, exp) in an object. The changes here are a step towards this idea.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, do you want to merge this, and then I'll review those changes in another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, if you're okay with the acceptance tests (second screenshot).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah looks good. Although might be worth a test with some negatives as well?

@krlmlr
Copy link
Member Author

krlmlr commented May 30, 2017

Negatives don't work in sci notation yet, not even in master (the third example is supposed to have negative numbers):

screenshot from 2017-05-30 23-08-19

@krlmlr
Copy link
Member Author

krlmlr commented May 30, 2017

Do you prefer merging or squashing for this repo?

@krlmlr
Copy link
Member Author

krlmlr commented May 30, 2017

Added colored output of test results to Travis: https://travis-ci.org/krlmlr/colformat/builds/237694396#L1304

Could you please enable Travis for the main repo?

@hadley
Copy link
Member

hadley commented May 31, 2017

Ooops.

I prefer squashing in general, unless you've already carefully rebase to make a clean history.

Travis is now on.

@krlmlr krlmlr merged commit fa9b098 into r-lib:master May 31, 2017
@krlmlr krlmlr deleted the f-refactor-3 branch May 31, 2017 13:33
@krlmlr
Copy link
Member Author

krlmlr commented May 31, 2017

Thanks. Squashing would be a tad easier if this was the default option.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants