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

Monetary columns and locale sensitive number display #5794

Open
wants to merge 21 commits into
base: development
Choose a base branch
from

Conversation

JorisGoosen
Copy link
Contributor

@JorisGoosen JorisGoosen commented Feb 19, 2025

This PR implements https://github.com/jasp-stats/INTERNAL-jasp/issues/1744 where a "monetary" format is added.
The format can just be "monetary" and then by default euro is chosen, to add a currency just do something like "monetary:USD" or "monetary:EUR".

This also implements almost everything in jasp-stats/jasp-issues#39 as far as localizing numbers and money go.

Date, time and or datetime colums are not added in this pr but the localisation stuff added now should help implementing it.

Ill also trigger some builds

Copy link
Contributor

@shun2wang shun2wang left a comment

Choose a reason for hiding this comment

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

In addition to two small suggested changes, there are other potentially serious problems:

  1. Label-Value combination change is not synchronously? when you change the locale, see that combination it's different.
  2. Edit value with decimal will crash JASP, error msg: replaceDoublesTillLabelsRowWithLabels choked on a temp-label that cant be converted to double???
    3.While import SPPS_Debug.sav in NumericRestrictedOrdinal from debug.sav the decimal point mixed both . and ,.
  3. Maybe setting options should be moved to "Result" and put it with Table options together.

@@ -3,30 +3,44 @@

Copy link
Contributor

@shun2wang shun2wang Feb 20, 2025

Choose a reason for hiding this comment

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

Suggested change
#include <functional>

I cannot compiling it without this with MSVC, and buildBot on Windows also failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ill have a look

@koenderks koenderks requested a review from shun2wang February 20, 2025 09:02
Copy link
Contributor

@koenderks koenderks left a comment

Choose a reason for hiding this comment

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

Took me a while to realize that I had to put in format = "monetary" instead of type = "monetary but after that it looks quite nice. Except for the fact that Euro's (at least in most European countries) uses , as the decimal separator and . as the thousand separator. Furthermore it looks nice.

image

@koenderks
Copy link
Contributor

@shun2wang Sorry, I accidentally requested a re-review from you. You can ignore this.

@EJWagenmakers
Copy link
Contributor

Yes but do we even want to allow comma's as digital separators? Maybe we should, but it sseems like it could be terribly confusing

@JorisGoosen
Copy link
Contributor Author

Took me a while to realize that I had to put in format = "monetary" instead of type = "monetary but after that it looks quite nice. Except for the fact that Euro's (at least in most European countries) uses , as the decimal separator and . as the thousand separator. Furthermore it looks nice.

Yeah so the dots/commas is dependent on locale not on the currency. Because otherwise you would get different formatting for lets say, a column of yen next dollars next to euros? Seems unwanted

@JorisGoosen
Copy link
Contributor Author

Yes but do we even want to allow comma's as digital separators? Maybe we should, but it sseems like it could be terribly confusing

Yeah this is a more broad question actually, also see the following:
French:
image
German:
image
American:
image

This adding of thousands-separators is a sideeffect of making the numbers locale dependent.
I think it looks pretty good to be honest.

I also added a "Use American English locale" for numbers checkbox in the settings.
This will do almost the same as what JASP always did (and the same as the english setting) but it does add these thousands separators.

Although if we dont want those maybe I can find some way to turn them off?

@JorisGoosen
Copy link
Contributor Author

In addition to two small suggested changes, there are other potentially serious problems:

1. Label-Value combination change is not synchronously? when you change the locale, see that combination it's different.

Well, the label is a stringified version of the value, so it has the locale dependent setting.
I setup the code in such a way that after a locale change it checks all the labels and converts them.
Not sure why this isnt working for you.

2. Edit value with decimal will crash JASP, error msg: `replaceDoublesTillLabelsRowWithLabels choked on a temp-label that cant be converted to double???`

Editing it in the labeleditor or in the data?

   3.While import [SPPS_Debug.sav](https://static.jasp-stats.org/debugData/SPPS_Debug.sav) in `NumericRestrictedOrdinal` from debug.sav the decimal point mixed both `.` and `,`.

Hey that is a good one, ill have a look.

3. Maybe setting options should be moved to "Result" and put it with `Table options` together.

The language option?
the locale/language also has an effect on the qml components (textinput etc) and the dataset.

@shun2wang
Copy link
Contributor

Editing it in the labeleditor or in the data?

In the data.

@JorisGoosen
Copy link
Contributor Author

Editing it in the labeleditor or in the data?

In the data.

Hmm, doesnt crash for me on linux, but Ill check later on a windows system

@shun2wang
Copy link
Contributor

shun2wang commented Feb 20, 2025

So you can see the crash also on Linux Debug build while press Enter
录屏 2025-02-20 22-23-52.webm

@JorisGoosen
Copy link
Contributor Author

So you can see the crash also on Linux Debug build while press Enter 录屏 2025-02-20 22-23-52.webm

Ah yes, now I can reproduce...

@JorisGoosen
Copy link
Contributor Author

I think Ive fixed the crash shun encountered.

Ive also made the "alternative" locale option a bit more comprehensive, like crazy comprehensive :p

There are new builds running, where the default locale used is the one linked to the chosen language, unless the alternative is selected:
image
image
image
image
image
image

@JorisGoosen
Copy link
Contributor Author

Wel Ive also changed the sorting of the languages and territories a bit, although it will be missing from (some?) of the builds.

@JorisGoosen
Copy link
Contributor Author

Ok, so now the alternative locale actually gets stored...
So Ive started new builds anyway.

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.

4 participants