Skip to content

Fix for regression in Node2Code.#11373

Merged
aparajit-pratap merged 3 commits intoDynamoDS:masterfrom
aparajit-pratap:fixN2C
Dec 19, 2020
Merged

Fix for regression in Node2Code.#11373
aparajit-pratap merged 3 commits intoDynamoDS:masterfrom
aparajit-pratap:fixN2C

Conversation

@aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Dec 18, 2020 •
edited
Loading

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 •
edited
Loading

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

mjkkirschner
mjkkirschner reviewed Dec 18, 2020
View reviewed changes
src/DynamoCore/Engine/ShortestQualifiedNameReplacer.cs
if (strIdentList.Length == 1)
{
return new IdentifierNode(strIdentList[0]);
return node;
Copy link
Member

@mjkkirschner mjkkirschner Dec 18, 2020

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

@aparajit-pratap aparajit-pratap Dec 18, 2020

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

@mjkkirschner mjkkirschner Dec 18, 2020

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

@aparajit-pratap aparajit-pratap Dec 18, 2020

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
mmisol
mmisol approved these changes Dec 18, 2020
View reviewed changes
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

Reviewers

@mjkkirschner mjkkirschner mjkkirschner left review comments

+1 more reviewer

@mmisol mmisol mmisol approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

WIP

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aparajit-pratap @mjkkirschner @mmisol