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

Fix type checking for integers on IsOutput serialization #8810

Merged
merged 3 commits into from
May 18, 2018

Conversation

saintentropy
Copy link
Contributor

Purpose

The purpose of this PR is to correct a bug in the serialization of nodes using the IsOutput UI. Previously nodes with a return type of integer were being set as Unknown. This issue is tracked with
QNTM-4085 and is a dependency of QNTM-3458.

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 @ramramps

FYIs

@nonoesp

@mjkkirschner
Copy link
Member

@saintentropy can you also fix this in the inputNodeData class - will it matter there?

@jnealb
Copy link
Collaborator

jnealb commented Apr 30, 2018

@saintentropy @mjkkirschner @smangarole @ramramps @aparajit-pratap I have to ask one last time: is this code flaw possibly anywhere else?

@mjkkirschner
Copy link
Member

InputNodeData.cs

@saintentropy
Copy link
Contributor Author

saintentropy commented Apr 30, 2018

NodeInputData.cs has the same enum types here... The reason it wasn't required is that we type the integer slider with an int here in IntegerSlider.cs and here in Integer.cs. Does that type need to be something else? The value gets serialized as an Int32

@jnealb
Copy link
Collaborator

jnealb commented May 1, 2018

Our integer type is int64 in 2.0

@mjkkirschner
Copy link
Member

yeah, I think it might be an improvement to change slider to int64.

@saintentropy
Copy link
Contributor Author

@mjkkirschner... Let's do this. Let me change the enum of supported types on NodeInputData.cs so that we are covered on type checking regardless of what happens on the Slider.

@QilongTang
Copy link
Contributor

@saintentropy Do you mind adding unit test, and you are good to go for 2.0.1, let us know because code freeze for 2.0.1 is by end of Inception Sprint

@saintentropy
Copy link
Contributor Author

@QilongTang Test added :-)

@QilongTang
Copy link
Contributor

@saintentropy Hi, I saw test files added but no unit tests code change, did we miss some commits here?

@saintentropy
Copy link
Contributor Author

@QilongTang This code is validated via the serialization tests.

@jnealb
Copy link
Collaborator

jnealb commented May 10, 2018

@saintentropy @QilongTang @Racel @smangarole @mjkkirschner Note that despite the mention of 2.0.1 above the JIRA task QNTM-4085 is marked for 2.1. Is this correct?

@QilongTang
Copy link
Contributor

@saintentropy Thank you for the confirmation, changes LGTM

@jnealb That is correct, I removed the 2.0.1 tag of this PR. So this will be a 2.1 issue only

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