Skip to content

QNTM-5580 - Correct Typo in the Dynamo json serialization to match CoGS API #9170

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.

Sign up for GitHub

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

Jump to bottom
Merged
saintentropy merged 6 commits into from
Oct 15, 2018
Merged

QNTM-5580 - Correct Typo in the Dynamo json serialization to match CoGS API #9170

saintentropy merged 6 commits into from
Oct 15, 2018

Conversation

@saintentropy
Copy link
Contributor

@saintentropy saintentropy commented Oct 11, 2018 •
edited
Loading

Purpose

The purpose of this PR is to correct a typo in the Dynamo serialization of the Outputs property as defined in the CoGS spec. Currently the InitialValue property is written as IntitialValue. This PR corrects that issue so Dynamo will serialize the Outputs json in the Dynamo file per the CoGS spec. Note IntitialValue is a public property so it remains in the node but is now marked as Obsolete.

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

@mjkkirschner @alfarok

FYIs

@sm6srw @QilongTang @nonoesp

@mjkkirschner
Copy link
Member

mjkkirschner commented Oct 11, 2018

It appears to me this this PR is using c# 7.0 features. I do not think thats recommended at this time as we have not all updated to 2017 at this point - the build targets 4.5.2 and won't even build without older sdks present. I would use c#6 max and update to 7.0 features if we update to 4.6 or 4.7 target later. I will be looking at that soon (or someone on Plaid will)

@mjkkirschner
Copy link
Member

mjkkirschner commented Oct 11, 2018

please note the failing builds.

nonoesp
nonoesp reviewed Oct 12, 2018
View reviewed changes
Copy link
Contributor

@nonoesp nonoesp left a comment

Choose a reason for hiding this comment

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

This is great! Looks good.

@mjkkirschner
Copy link
Member

mjkkirschner commented Oct 12, 2018

@saintentropy whats with the failing engops build?

@saintentropy
Copy link
Contributor Author

saintentropy commented Oct 12, 2018

@mjkkirschner engops build passes

@mjkkirschner
Copy link
Member

mjkkirschner commented Oct 15, 2018

@saintentropy this looks good to me.

@saintentropy saintentropy mentioned this pull request Oct 15, 2018
Merged
7 tasks
@saintentropy saintentropy merged commit 507c402 into DynamoDS:master Oct 15, 2018
saintentropy added a commit to saintentropy/Dynamo that referenced this pull request Oct 22, 2018
@saintentropy
QNTM-5580 - Correct Typo in the Dynamo json serialization to match Co…
9ae3372 
…GS API (DynamoDS#9170)

* Fix typo

* Obsolete the existing property

* Update test

* remove expression body syntax

* Add serialization test with typo
@saintentropy saintentropy mentioned this pull request Oct 22, 2018
Merged
7 tasks
saintentropy added a commit that referenced this pull request Oct 24, 2018
@saintentropy
Cherry Picks for Refinery support in Dynamo 2.0.2 (#9198)
4ecc983 
* 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
@mjkkirschner mjkkirschner mentioned this pull request Feb 25, 2019
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nonoesp nonoesp nonoesp left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@saintentropy @mjkkirschner @nonoesp