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 for regression in Node2Code. #11373

Merged
merged 3 commits into from
Dec 19, 2020

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Dec 18, 2020

Purpose

Fix for regression in Node2Code.

n2c

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.
  • This PR modifies some build requirements and the readme is updated

FYIs

@saintentropy

@mjkkirschner
Copy link
Member

mjkkirschner commented Dec 18, 2020

@aparajit-pratap thanks for fixing this so quickly,
do you think we can add a test case for + or a similar class of errors?
didn't see your note above.

@@ -106,7 +106,7 @@ public override AssociativeNode VisitIdentifierNode(IdentifierNode node)
// return IdentifierNode if identifier string is not separated by '.'
if (strIdentList.Length == 1)
{
return new IdentifierNode(strIdentList[0]);
return node;
Copy link
Member

Choose a reason for hiding this comment

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

If my memory serves correctly, then the AST walkers are a bit inconsistent when it comes to returning new AST nodes or the same node - maybe this should return a copy of the node instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AST walkers are meant to return new nodes only if they are rewritten and return the original node if there is no change.

Copy link
Member

Choose a reason for hiding this comment

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

ah! Is that documented somewhere? Would it make sense to add a note about that to the base class or something? Not that it needs to be part of this pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do.

@aparajit-pratap aparajit-pratap changed the title Fix to regression in Node2Code. Fix for regression in Node2Code. Dec 18, 2020
Copy link
Collaborator

@mmisol mmisol left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for fixing @aparajit-pratap !

@aparajit-pratap aparajit-pratap merged commit 32cb182 into DynamoDS:master Dec 19, 2020
@aparajit-pratap aparajit-pratap deleted the fixN2C branch December 19, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants