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

Enable package node migration for Dynamo Core 2.0+ graphs #9306

Merged
merged 17 commits into from Dec 17, 2018

Conversation

Projects
None yet
4 participants
@alfarok
Copy link
Contributor

alfarok commented Dec 11, 2018

Purpose

QNTM-5547

This PR restores the previous (1.3 and older) behavior for package node migration for JSON graphs (2.0+). For example, you can now migrate a packaged node that was built against Core 2.0 but was later renamed. We previously handled 1.3 (XML) to 2.0 (JSON) but never 2.0 to 2.0+.

Package Migration Strategies:
ZeroTouch nodes => Use a Migrations.XML file located in the packages bin folder
NodeModel derived nodes => Use the AlsoKnownAs attribute on the class

Examples:

MyZeroTouchLib.MyNodes.SayHello to MyZeroTouchLib.MyNodes.SayHelloRENAMED

<?xml version="1.0"?>
<migrations>
  <priorNameHint>
    <oldName>MyZeroTouchLib.MyNodes.SayHello</oldName>
    <newName>MyZeroTouchLib.MyNodes.SayHelloRENAMED</newName>
  </priorNameHint>
</migrations>

SampleLibraryUI.Examples.DropDownExample to SampleLibraryUI.Examples.DropDownExampleRENAMED

namespace SampleLibraryUI.Examples
{
    [NodeName("Drop Down Example")]
    [NodeDescription("An example drop down node.")]
    [IsDesignScriptCompatible]
    [AlsoKnownAs("SampleLibraryUI.Examples.DropDownExample")]
    public class DropDownExampleRENAMED : DSDropDownBase
    {
        ...
    }
{

Testing

  1. Created a test package with several node types (not committed)
  2. Created a legacy graph including these nodes (committed)
  3. Renamed the classes of all included nodes
  4. Included migrations.xml for ZeroTouch nodes
  5. Added [AlsoKnownAs] attribute for the NodeModel node
  6. Rebuilt package (committed)
  7. Assert all nodes migrate to the appropriate types and no dummy nodes are found

Test Cases:

  • ZeroTouch single input (overload)
  • ZeroTouch multi input (overload)
  • ZeroTouch no input multi output
  • NodeModel dropdown

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

@mjkkirschner @aparajit-pratap @QilongTang

FYIs

@johnpierson

var priorNames = libraryServices.GetPriorNames();
FunctionDescriptor functionDescriptor;

// Attempt to located a newer migrated version of the node

This comment has been minimized.

Copy link
@mjkkirschner

mjkkirschner Dec 11, 2018

Member

sic(locate)?

FunctionDescriptor functionDescriptor;

// Attempt to located a newer migrated version of the node
if (priorNames.ContainsKey(mangledName))

This comment has been minimized.

Copy link
@aparajit-pratap

aparajit-pratap Dec 11, 2018

Contributor

Just use TryGetValue . You can check for the key and get the value at the key at the same time.

alfarok added some commits Dec 12, 2018

@alfarok alfarok changed the title Enable package node migration using a migrations.xml for core 2.0+ graphs Enable package node migration for Dynamo Core 2.0+ graphs Dec 13, 2018

public ElementResolver ElementResolver { get; set; }
//map of all loaded assemblies including LoadFrom context assemblies
private Dictionary<string, List<Assembly>> loadedAssemblies;

[Obsolete("This function will be removed in Dynamo 3.0, please use new NodeReadConverter method with additional parameters to support node migration.")]

This comment has been minimized.

Copy link
@mjkkirschner

mjkkirschner Dec 14, 2018

Member

constructor

@alfarok alfarok added the PTAL label Dec 14, 2018

@alfarok

This comment has been minimized.

Copy link
Contributor Author

alfarok commented Dec 14, 2018

@mjkkirschner @aparajit-pratap Hey guys, I think this one is ready for a final look when you get a free moment.

@QilongTang

This comment has been minimized.

Copy link
Contributor

QilongTang commented Dec 14, 2018

@alfarok Question on nodeModel nodes renaming, instead of dong what you put as example, can I rename the node in the opposite way below?

namespace SampleLibraryUI.Examples
{
    [NodeName("Drop Down Example")]
    [NodeDescription("An example drop down node.")]
    [IsDesignScriptCompatible]
    [AlsoKnownAs("SampleLibraryUI.Examples.DropDownExampleRENAMED ")]
    public class DropDownExample : DSDropDownBase
    {
        ...
    }
{
@mjkkirschner

This comment has been minimized.

Copy link
Member

mjkkirschner commented Dec 14, 2018

@alfarok did you ever investigate if you could get rid of this

          else if (type == typeof(DSVarArgFunction))
            {
                var functionId = Guid.Parse(obj["FunctionSignature"].Value<string>());
                node = manager.CreateCustomNodeInstance(functionId);
            }

?

@mjkkirschner

This comment has been minimized.

Copy link
Member

mjkkirschner commented Dec 14, 2018

I think some tests will break as they count the number of packages loaded from the test/pkgs folder.

@alfarok

This comment has been minimized.

Copy link
Contributor Author

alfarok commented Dec 14, 2018

@QilongTang I haven't tried that but my assumption is that it could be used to migrate the node in the opposite direction? I think it makes more sense for the classes to be renamed so nodes can more clearly be traced back to their origin and keeps the 2 in sync. The library basically does the opposite when it was reorganized and now none of the locations truly represent the code locations/classes.

@alfarok

This comment has been minimized.

Copy link
Contributor Author

alfarok commented Dec 14, 2018

@mjkkirschner I don't think it is required but am also a bit nervous about removing it thinking it may have been added for some strange edge case? What do you think?

Also the self-serve is running one last time so I will update any tests that rely of the package count if required.

@QilongTang

This comment has been minimized.

Copy link
Contributor

QilongTang commented Dec 14, 2018

@alfarok Good to know! I think people need to be aware of this is the preferred way

this.manager = manager;
this.libraryServices = libraryServices;
this.nodeFactory = nodeFactory;
this.isTestMode = isTestMode;

This comment has been minimized.

Copy link
@aparajit-pratap

aparajit-pratap Dec 14, 2018

Contributor

Minor but I think we avoid using this as per existing coding conventions (which may have been forgotten along the way). Also if you use Resharper you'll see that a lot of these things are highlighted by the tool.

This comment has been minimized.

Copy link
@alfarok

alfarok Dec 14, 2018

Author Contributor

This is the old code and this file in particular is filled with this. Removing it in certain areas creates ambiguity between constructor parameters and class properties, such as above

nodeFactory.ResolveType(unresolvedName, out newType);

if (newType != null) // If resolved update the type
{ type = newType; }

This comment has been minimized.

Copy link
@aparajit-pratap

aparajit-pratap Dec 14, 2018

Contributor

Here it is preferred to have a single curly brace on a line (don't know what happened to coding conventions once followed by the team):

// If resolved update the type
if (newType != null) 
{ 
type = newType; 
}

This comment has been minimized.

This comment has been minimized.

Copy link
@QilongTang

QilongTang Dec 14, 2018

Contributor

oh wow, if we would like to get this topic up, please fix all the comments in this PR so they are in the same format 😉
//[space][Comment]

This comment has been minimized.

Copy link
@alfarok

alfarok Dec 14, 2018

Author Contributor

Sorry comments without a space were copied from the constructor I made obsolete, I would never commit such a crime 😂

@aparajit-pratap

This comment has been minimized.

Copy link
Contributor

aparajit-pratap commented Dec 14, 2018

@alfarok other than the harping about coding conventions this looks good to me. Thanks!

@alfarok

This comment has been minimized.

Copy link
Contributor Author

alfarok commented Dec 14, 2018

@aparajit-pratap 🚨 🚓 guilty as charged, I don't think I've ever seen that doc before though

alfarok added some commits Dec 14, 2018

@mjkkirschner

This comment has been minimized.

Copy link
Member

mjkkirschner commented Dec 17, 2018

LGTM

@mjkkirschner mjkkirschner added LGTM and removed PTAL labels Dec 17, 2018

@alfarok alfarok merged commit 58a4361 into DynamoDS:master Dec 17, 2018

3 checks passed

EngOps Build EngOps Build Succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

alfarok added a commit to alfarok/Dynamo that referenced this pull request Dec 17, 2018

@alfarok alfarok referenced this pull request Dec 17, 2018

Merged

increment package count in test #9323

1 of 7 tasks complete

alfarok added a commit that referenced this pull request Dec 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.