QNTM-5568 Refinery needs to support culture invariance for number fmt, part Deux #9178

sm6srw · 2018-10-15T15:27:54Z

Purpose

This pull request addresses JIRA task QNTM-5568.

This pull request does

format the value string in output nodes using the InvariantCulture format
use the InvariantCulture format when converting the value string in input and output nodes to double
Added tests for InputData and OutputData
Added test for loading and comparing dyn files while being in a different culture.

Declarations

Check these if you believe they are true

The code base 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.

Reviewers

@longc @mjkkirschner

FYIs

mjkkirschner · 2018-10-15T15:56:19Z

This looks good, do you intend on adding localized tests?
Also will you take advantage of @saintentropy 's new property?

sm6srw · 2018-10-15T16:05:15Z

Yes, I can try to add tests for it. Do we have a something similar somewhere?
@saintentropy What new property?

saintentropy · 2018-10-15T16:35:09Z

He means my #9170

sm6srw · 2018-10-15T17:12:13Z

Ah, so that was the spelling thing. I didn't even notice!

saintentropy · 2018-10-15T18:20:01Z

LGTM @sm6srw

sm6srw · 2018-10-15T19:28:47Z

@mjkkirschner I added some tests. PTAL.

mjkkirschner · 2018-10-15T19:30:11Z

test/DynamoCoreTests/NodeToCodeTest.cs

+
+        [Test]
+        [Category("RegressionTests")]
+        public void TestNodeOutputDataInDifferentCulture()


@sm6srw are these node2code tests?

Well, that's where I found other similar tests. Do you want me to move these somewhere else?

this seems like the wrong location - node2Code has to do with using the node to designscript feature.

I would put this in tests related to inputOutputData etc.

Make sense, thanks!

mjkkirschner · 2018-10-15T19:33:50Z

test/DynamoCoreTests/SerializationTests.cs

+                serializationTestUtils.SaveWorkspaceComparisonData);
+
+            Thread.CurrentThread.CurrentCulture = currentCulture;
+            Thread.CurrentThread.CurrentUICulture = currentUICulture;


does this actually set the culture back to the correct culture? could we assert it?

Yes it does. But I added some asserts just in case.

I guess I meant does this actually set it back to en-us or something - I might be totally confused but this line var currentCulture = Thread.CurrentThread.CurrentCulture;
looks like it might just make a reference to the current Culture so when you set the culture to the french culture it seems that it would also update.

Is the currentCulture a value type or reference type?

Yes, it sets it back to what was there before, in our case en-us. It must be value type because the switching works (verified in the debugger).

test/DynamoCoreTests/SerializationTests.cs

sm6srw · 2018-10-16T15:36:04Z

@mjkkirschner PTAL when you have a moment.

sm6srw · 2018-10-17T12:36:34Z

@mjkkirschner PTAFL!

mjkkirschner · 2018-10-17T12:43:27Z

Lgtm

…

On Oct 17, 2018, at 8:36 AM, Jorgen Dahl ***@***.***> wrote: @mjkkirschner PTAFL! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

…, part Deux (DynamoDS#9178) * Don't culture format value strings. * Assume value strings are formated with `InvariantCulture` * Add tests * More work on tests * Clarify switching of cultures

* Add RegisterFroTrace attribute as a trigger in Json serialization of trace * PR comment * [ 2.0 Release Cut ] Bump Up Master Patch Version (#8647) * Bump Up Patch Version * Add version * Update comments and version baseline * Address Comments * CubicMillimeterToCubicMeter conversion * Revert "CubicMillimeter To CubicMeter conversion" * Fix the units bug * Remove thread sleep * missed the test * Fix static property compilation and crash (#8649) * fix crash, compilation of static property * code cleanup * add test * add test graph * QNTM-3646: Fix creation of unwanted input port for list index-assignment (#8648) * fix for additional input port * fixes for output ports * update tests * added test, code cleanup * address review comments * QNTM-3279: Fix for crash when trying to directly call properties on class name (#8661) * fix for compiling property method as function pointer * code cleanup * add tests * Cherry-pick #8663 (#8664) * Update failing tests (#8663) * update tests * Merge LibG binaries with Geometry.Approximate API (#8666) * merge LibG binaries with Geometry.Approximate() * Update license * scroll bar fix in incanvas search * QNTM-3641: Prevent ColorRange node from crashing with old style List syntax (#8670) (#8674) * fix crash with ColorRange1D node * add test * add test set unsaved changes false when initializing json custom node * Update changes from librarie.js to fix QNTM-3710 (#8678) After investigating the errors in the EngOps check it was thought that these changes could not have caused the current failures. Other PRs have also shown the same issues. Given all of that, this PR is being merged. * address comments * Update LibG binaries for ASM224 branch (#8681) * update LibG binaries for ASM224 branch * PR comment * QNTM-3841 Add geometry creation option to Dynamo CLI (#8775) * POC * Cleanup code. * Add command line option -g for generating geometry json file. * Remove filtering * Fix node state flags at load time. * Use UpdateValue for setting isVisible on node model. Revert set to private. * Remove filtering. * Clean up holder code. * Cleanup converter code. Use node model defaults as starting point. * Preload the same libraries for CLI as for Sandbox. * Add color support * Simplify json * Update file format. * Remove property expressions. * Restore comment. * Restore Cef Settings * Add unit test for CLI geometry option. * Remove comment. * Don't extend CommandLineRunner. * Use var when possible. * Remove commented out code. * Use DynamoViewModel in CLI. * Use DefaultWatch3DViewModel for CLI. * Add new project DynamoWPFCLI. * Let CommandLineRunnerWPF inherit from CommandLineRunner. * Fix review comments. * Fix unstable unit tests. * Remove Dispose method. * Add comments * Fix type check * Apply same fix to NodeInputData * Add serialization tests * Fix typo in Extansion. * QNTM-4278 DynamoWPFCLI should use CoGs geometry format (#8887) * POC * Cleanup code. * Add command line option -g for generating geometry json file. * Remove filtering * Fix node state flags at load time. * Use UpdateValue for setting isVisible on node model. Revert set to private. * Remove filtering. * Clean up holder code. * Cleanup converter code. Use node model defaults as starting point. * Preload the same libraries for CLI as for Sandbox. * Add color support * Simplify json * Update file format. * Remove property expressions. * Restore comment. * Restore Cef Settings * Add unit test for CLI geometry option. * Remove comment. * Don't extend CommandLineRunner. * Use var when possible. * Remove commented out code. * Use DynamoViewModel in CLI. * Use DefaultWatch3DViewModel for CLI. * Add new project DynamoWPFCLI. * Let CommandLineRunnerWPF inherit from CommandLineRunner. * Fix review comments. * Fix unstable unit tests. * Remove Dispose method. * Add comments * First pass * Formatting fixes. * Remove XML stuff. * Some final adjustments. * Add more comments. * Move comment. * Only store the geometry array. * Refactor. * Add interface * Make interface internal * Rename implmentation. * Allow internal access for CLI * Move default implementation to core. * Make setters private * Address review comments. * match Revit CEF intitializtion settings * match Revit CEF intitializtion settings * match Revit CEF intitializtion settings * QNTM-5580 - Correct Typo in the Dynamo json serialization to match CoGS API (#9170) * Fix typo * Obsolete the existing property * Update test * remove expression body syntax * Add serialization test with typo * AVP NumberInputNode deserialization * add testing * QNTM-5568 Refinery needs to support culture invariance for number fmt, part Deux (#9178) * Don't culture format value strings. * Assume value strings are formated with `InvariantCulture` * Add tests * More work on tests * Clarify switching of cultures

sm6srw added 2 commits October 15, 2018 11:15

Don't culture format value strings.

c1c2fad

Assume value strings are formated with InvariantCulture

03d0006

Merge remote-tracking branch 'upstream/master' into QNTM-5568

e3fdb7b

saintentropy closed this Oct 15, 2018

saintentropy reopened this Oct 15, 2018

Add tests

054b262

mjkkirschner reviewed Oct 15, 2018

View reviewed changes

test/DynamoCoreTests/SerializationTests.cs Outdated Show resolved Hide resolved

More work on tests

43d8729

Clarify switching of cultures

35c116a

sm6srw merged commit abc6d62 into DynamoDS:master Oct 17, 2018

sm6srw deleted the QNTM-5568 branch October 17, 2018 12:51

saintentropy mentioned this pull request Oct 22, 2018

Cherry Picks for Refinery support in Dynamo 2.0.2 #9198

Merged

7 tasks

QNTM-5568 Refinery needs to support culture invariance for number fmt, part Deux #9178

QNTM-5568 Refinery needs to support culture invariance for number fmt, part Deux #9178

Uh oh!

Conversation

sm6srw commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

This pull request does

Declarations

Reviewers

FYIs

Uh oh!

mjkkirschner commented Oct 15, 2018

Uh oh!

sm6srw commented Oct 15, 2018

Uh oh!

saintentropy commented Oct 15, 2018

Uh oh!

sm6srw commented Oct 15, 2018

Uh oh!

saintentropy commented Oct 15, 2018

Uh oh!

sm6srw commented Oct 15, 2018

Uh oh!

mjkkirschner Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

sm6srw Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

sm6srw Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

sm6srw Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

sm6srw Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sm6srw commented Oct 16, 2018

Uh oh!

sm6srw commented Oct 17, 2018

Uh oh!

mjkkirschner commented Oct 17, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sm6srw commented Oct 15, 2018 •

edited

Loading

sm6srw Oct 15, 2018 •

edited

Loading

sm6srw Oct 16, 2018 •

edited

Loading