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

DYN-6526 Converter Culture Bug Swedish #15587

Conversation

RobertGlobant20
Copy link
Contributor

Purpose

Fixing Culture in Converter used in the Number node.
In Civil3D when loading a dyn file that contains a Number with value 0.25 the converter was converting the value to 25.00 showing this value in the Number node (due that the CurrentCulture is "sv-FI"). For fixing this I just set the InvariantCulture in the Converter.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

Fixing Culture in Converter used in the Number node.

Reviewers

@QilongTang

FYIs

When loading a dyn file that contains a Number with value 0.25 the converter was converting the value to 25.00 showing this value in the Number node. For fixing this I just set the InvariantCulture in the Converter.
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-6526

@RobertGlobant20
Copy link
Contributor Author

GIF showing the behavior BEFORE my fix
acad_k9AZfobbBO

@RobertGlobant20
Copy link
Contributor Author

GIF showing the behavior AFTER my fix.
acad_mo5anRIq22

Copy link

github-actions bot commented Nov 1, 2024

UI Smoke Tests

Test: success. 11 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests

@QilongTang
Copy link
Contributor

QilongTang commented Nov 4, 2024

Would you add a test or update a test? hmm thinking about this a bit more, seems a unit test would not make a diff because the output expectation was related to system culture.. Maybe this is good for now.

Do we also only display number without , but . on the UI? e.g. 3.4 would be 3,4 from some locale. If that's the case, this change should be good

@QilongTang QilongTang added this to the 3.4 milestone Nov 4, 2024
@mjkkirschner
Copy link
Member

I find this change kind of strange... it seems like you are overriding the purpose of this converter - maybe it shouldn't exist at all anymore?

Also you can test it by changing the thread culture and then changing it back, I think there are other tests like this.

@mjkkirschner mjkkirschner reopened this Nov 4, 2024
@mjkkirschner
Copy link
Member

@RobertGlobant20 sorry - didn't mean to close, hit the wrong button.

@RobertGlobant20
Copy link
Contributor Author

@QilongTang I think we are not supporting creating unit test in other languages.
For the case of the "," that you were talking about, do you remember under which locales we are showing the "," char?
At least for the Number node in čeština(czech), Swedish language we are not showing the "," p

Would you add a test or update a test? hmm thinking about this a bit more, seems a unit test would not make a diff because the output expectation was related to system culture.. Maybe this is good for now.

Do we also only display number without , but . on the UI? e.g. 3.4 would be 3,4 from some locale. If that's the case, this change should be good

@QilongTang as we discussed in the standup I cannot add a unit test for Dynamo in another language.
For the case of the "," in the UI, I´ve tested (e language = Czech - Swedish OS ) and is serializing and showing the numbers in the same way the user inserted the info (using the point), for example I added a Number with a large number and is serialized in the next way:
image

For the other case if the language = Czech (Swedish OS) consider that we have the value "0.25 " in the Number node If the user inserts a "," instead of "." is replaced automatically by "." and the new value is "250.000" (and is serialized in the same way).
image
image

Let me know if is the expected behavior

@RobertGlobant20
Copy link
Contributor Author

I find this change kind of strange... it seems like you are overriding the purpose of this converter - maybe it shouldn't exist at all anymore?

Also you can test it by changing the thread culture and then changing it back, I think there are other tests like this.

@mjkkirschner correct me if I´m wrong but the converter is adding the PrecisionFormat to the string so for example if you enter "2.5" and the NumberFormat in Preferences is 0.000 then will be showing the value "2.500", then I guess it should exist.
image
image

@mjkkirschner
Copy link
Member

@RobertGlobant20 that makes sense, missed the precision format.

@QilongTang QilongTang merged commit eee208c into DynamoDS:master Nov 5, 2024
38 checks passed
@QilongTang
Copy link
Contributor

@RobertGlobant20 Please cherry-pick

@mjkkirschner
Copy link
Member

@QilongTang @RobertGlobant20 can you explain why changing the thread culture does not work for creating a test?

@RobertGlobant20
Copy link
Contributor Author

@QilongTang @RobertGlobant20 can you explain why changing the thread culture does not work for creating a test?

@mjkkirschner for reproducing this particular case we need to have a OS in Sweedish Language (including Region and Region Format) and Dynamo for Civil3D (in Sandbox and Revit is not reproducible) should be in Czech language, was reported that for other languages like English or Spanish is not reproducible.

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.

3 participants