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

cherry pick -Avoid namespace collisions when collapsing graph to custom node (#9330) 2.1 RC #9360

Merged

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented Jan 3, 2019

Purpose

cherry pick:
#9330

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 - this looks much better now, matches the change amounts in the original PR.
I think what happened before was that the merge conflicts stopped the full list of commits from being applied, I'm not sure if you can continue a cherry pick on a conflicted squash commit like you can with regular commits - at least git did not tell me I needed to continue after the merge conflicts were fixed, so we should be cautious of cherry picking a squashed commit with conflicts atleast until we look into it a bit more.

…moDS#9330)

* this works - but I am hesitant to modify this replacer.
Not sure how this effects node2code. - may create rewriter directly.

* add a new visitor method for typedIdentifierNode
use it in custmNodeManager collapse method

* move shortQualifiedNameReplacer
update references
add comments to node2code mutation tests

* add more tests
add extra logic to deserialization for symbol node element resolver cache

* revert public API break, keep rewriter in dynamoCore but in new file

* remove extra usings added previously.

* remove debugging code
@mjkkirschner
Copy link
Member Author

I've also verified the 5 failing tests now pass locally on this branch.

@QilongTang
Copy link
Contributor

LGTM

@QilongTang
Copy link
Contributor

Given the good state it is, I am going to merge this together with #9361 because there are several API changes Revit team need to consume on their end

@QilongTang QilongTang added the LGTM Looks good to me label Jan 3, 2019
@QilongTang QilongTang merged commit 2351da5 into DynamoDS:RC2.1.0_master Jan 3, 2019
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