Skip to content

Conversation

@alfarok
Copy link
Contributor

@alfarok alfarok commented May 30, 2018

Purpose

This PR addresses the Dynamo deserialization of a NumberInput node that was originally serialized in AVP. The NumberInput node in AVP defines the InputValue property of type number. If the value is an integer (1, 1.0, 1.00, etc) the value is always serialized as 1 (without any decimals). Upon deserialization in Dynamo an error was being thrown while attempting to parse the expected double value. This change resolves the issue and enables proper deserialization/execution.

Testing

3 unit tests added verifying proper deserialization of number nodes containing values of 1, 1.0, 1.1

image

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

@QilongTang

FYIs

@ramramps

@QilongTang
Copy link
Contributor

LGTM

@mjkkirschner
Copy link
Member

test?

@alfarok
Copy link
Contributor Author

alfarok commented May 30, 2018

@mjkkirschner added test cases for deserialization of input values of 1, 1.0, 1.1 where 1 was originally failing

@QilongTang
Copy link
Contributor

@alfarok Thank you for doing that 😉

@alfarok alfarok merged commit d9f3ca2 into DynamoDS:master May 30, 2018
@jnealb
Copy link
Collaborator

jnealb commented Jun 6, 2018

@alfarok @mjkkirschner @QilongTang @mrahmaniasl @saintentropy @nonoesp @varvaratou Is there a JIRA task for this?

@alfarok
Copy link
Contributor Author

alfarok commented Jun 6, 2018

@jnealb it was originally part of QNTM-4022. The AC specified when an AVP graph is serialized, the model / view should represent the same properties as Dynamo. This couldn't be completed without this change in Dynamo. Do we need an additional testing task?

@jnealb
Copy link
Collaborator

jnealb commented Jun 6, 2018

@alfarok No, thanks, we just need to track the task for Refinery as the fix affects the project (and is not in 2.0.1)

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