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

Better decimal formatting #626

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

drvictorvs
Copy link

@drvictorvs drvictorvs commented Dec 22, 2022

Makes sure that pillar formats decimals according to the decimal mark defined by the user with the OutDec option (as described in help("options")).

I was in doubt whether I should have added any sort of validation to OutDec. I initially wrote it to default to . in EN locales and , in certain European locales, and to accept the OutDec value only if it was a character and length 1. I figured the length was especially important given how pillar deals with screen width. I assumed base would also have such checks. However, out of curiosity I set OutDec to "potato" to see how the base print function would deal with it, and indeed it printed 0.1 as [1] 0potato1. Why should I take that away from users?

So, I didn't add the length requirement, but I imagine you would like to do that, as well as add some sort of type validation. Regardless, the important thing is that pillar respects the OutDecoption.

Best regards.

Makes sure that pillar formats decimals according to the decimal mark defined by the user with the "OutDec" option (as described in help("options")).
@krlmlr
Copy link
Member

krlmlr commented Mar 12, 2023

Thanks.

@hadley: Should our printing depend on getOption("OutDec") ?

@hadley
Copy link
Member

hadley commented Mar 14, 2023

Hmmm, this seems like a change with potentially unexpected consequences, so we’d need to carefully analyse to know whether or not it’s worth.

@krlmlr
Copy link
Member

krlmlr commented Dec 15, 2024

I appreciate the contribution and apologize for being slow. I'm not really prepared to deal with the downstream consequences of this change, but I see how it can be useful.

Running revdepchecks now. If they come back without downstream problems, we can consider if we check (as you suggested) that the width is 1 and if we provide an option in pillar to override and restore the original behavior.

@krlmlr
Copy link
Member

krlmlr commented Dec 15, 2024

The results seem good. If you're still interested in this, happy to review an extension:

  • Fallback option to always use .
  • Tests
  • Documentation (perhaps in the tibble package)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants