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-2273 - Fix crash by compiling null input to null AST node #10127

Merged
merged 4 commits into from
Nov 13, 2019

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Nov 11, 2019

Purpose

JIRA: https://jira.autodesk.com/browse/DYN-2273

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.

Reviewers

@DynamoDS/dynamo

@QilongTang
Copy link
Contributor

@aparajit-pratap , my memory fading. Can you describe null node a bit?

@aparajit-pratap aparajit-pratap changed the title DYN-2273 - Fix crash by compiling null input to null node DYN-2273 - Fix crash by compiling null input to null AST node Nov 12, 2019
@aparajit-pratap
Copy link
Contributor Author

@QilongTang by nullnode, I meant Null AST node, sorry for not making that clear before. We have a special AST node to represent null. In the case where an input AST is null, we would rather propagate the null than throw an exception either at the time of AST compilation or at runtime, which was what was happening before.

@QilongTang
Copy link
Contributor

@aparajit-pratap Thank you, makes sense. LGTM

@QilongTang QilongTang added the LGTM Looks good to me label Nov 12, 2019
@aparajit-pratap aparajit-pratap merged commit 89465ff into DynamoDS:master Nov 13, 2019
@aparajit-pratap aparajit-pratap deleted the dyn-2273 branch November 13, 2019 14:41
QilongTang pushed a commit that referenced this pull request Feb 13, 2020
* fix crash by compiling null input to null node

* add test

* update recorded test to reflect null value of downstream node

* remove unused usings
QilongTang added a commit that referenced this pull request Feb 13, 2020
#10391)

* fix crash by compiling null input to null node

* add test

* update recorded test to reflect null value of downstream node

* remove unused usings

Co-authored-by: aparajit-pratap <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants