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

fix: #992 do not localize numbers in csv #1017

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hexaltation
Copy link
Collaborator

Fix #992

Numbers should not be localized.
French float numbers representation was "1,234" in csv now its 1.234.

It makes it simpler for many other softwares to guess correctly data type when importing csv generated by grist.

Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

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

Modulo the code formatting and the CI result, LGTM

@hexaltation
Copy link
Collaborator Author

hexaltation commented Jun 3, 2024

This draft was to run tests that I can't run locally.

Modulo the code formatting and the CI result, LGTM

Test fails are legitimate.

Replacing

   public _formatPlain(value: number): string {
-    return this._numFormat.format(value);
+    return String(value);
   }

leads to some wanted identities fail :

***
(1) ClientUnitTests.ntest should reach 100% with no failures
Error: Browser returned 6 failed tests:
ValueFormatter NumericFormatter should support plain format: AssertionError: expected 'Infinity' to equal '∞'
ValueFormatter NumericFormatter should support min/max decimals: AssertionError: expected '12' to equal '12.00'
ValueFormatter NumericFormatter should support thousand separators: AssertionError: expected '1000000' to equal '1,000,000.0000'
ValueFormatter NumericFormatter should support currency mode: AssertionError: expected '1000000' to equal '$1,000,000.00'
ValueFormatter NumericFormatter should support percentages: AssertionError: expected '0.5' to equal '50%'
ValueFormatter NumericFormatter should support scientific mode: AssertionError: expected '0.5' to equal '5E-1'
    at Context.<anonymous> (test/nbrowser/ClientUnitTests.ntest.js:21:13)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
***

@hexaltation
Copy link
Collaborator Author

I think that a better Idea than previous one is to force locals to en-US for decimal in Intl.NumberFormat() "decimal" (default)
(only ?) in case of dsv export.

@dsagal
Copy link
Member

dsagal commented Jun 3, 2024

I am not sure that your change affects only csv exports (does it not affect the main UI?) But even for CSV, see my comment here: #992 (comment). In short, it's an intentional choice to export numbers in the same format as what you see on the screen, and it's consistent with other produces (Excel/Google Sheets), and other operations (copy-pasting). If having an "unformatted" output like this is important, I think we'd need to provide an option for the user to choose the behavior.

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.

Number, i18n and CSV download
3 participants