Migrate number nodes with sequence to CBNs. #7582
Conversation
|
|
||
| // Increase the number values. | ||
| numNode.Value = numNodeValue; | ||
| numNode.SetCodeContent(numNodeValue + ";", new ProtoCore.Namespace.ElementResolver()); |
There was a problem hiding this comment.
Is this just a temporary change for testing purposes?
There was a problem hiding this comment.
No. This test needs to be updated to work after this migration is added.
There was a problem hiding this comment.
That's going to replace the existing test?
| @@ -321,22 +321,16 @@ orderby result.From | |||
| var migrationData = new NodeMigrationData(elNode.OwnerDocument); | |||
| migrationData.AppendNode(elNode as XmlElement); | |||
|
|
|||
| while (workspaceVersion != null && workspaceVersion < currentVersion) | |||
| foreach(var migration in migrations.Where(m=>m.From >= workspaceVersion)) | |||
There was a problem hiding this comment.
seems to make sense - does it break any existing migration tests here or for revit?
There was a problem hiding this comment.
though - From seems kind of strange in context when said out loud, this WorkspaceMigration migrates workspaces with versions from 2.0... I don't know how to finish this sentence to make this correct.
Perhaps WorkspaceAttribute needs a different property name like migrateWorkspaceVersionsBelow ... I am not sure.
Also seems CurrentVersion is no longer used in this method.
There was a problem hiding this comment.
I'll remove CurrentVersion, and I propose to rename from: to version, as in "This node migration is applied to nodes in version x.x.x and higher."
There was a problem hiding this comment.
Isn't it "version x.x.x and lower", or "lower than version x.x.x"?
There was a problem hiding this comment.
@gregmarr You are correct :)
| @@ -1360,6 +1354,7 @@ public class NodeMigrationAttribute : Attribute | |||
| /// <summary> | |||
| /// Version this migrates to. | |||
| /// </summary> | |||
| [Obsolete("All migrations are now processed, beginning with the first migration whose version is >= the workspace version. The To property will be ignored.", false)] | |||
There was a problem hiding this comment.
hmm, if this is targeted towards Dynamo 2.0 release should we just get rid of this? Or you prefer the tag for documentation purposes?
There was a problem hiding this comment.
That's open for discussion. I can just rip it out, and take out all references to to: as well. I started to do that yesterday and thought better of it. But we are making a major version change.
There was a problem hiding this comment.
I will need to do this in the Revit branches as well.
There was a problem hiding this comment.
There appears to be only one NodeMigration applied in the Revit repos: https://github.com/DynamoDS/DynamoRevit/search?utf8=✓&q=NodeMigration
There was a problem hiding this comment.
I'm not sure how these are used (it's been a while) but I just want to be sure that this change doesn't break them:
There was a problem hiding this comment.
I do not believe that Zero Touch migrations are affected, because they don't use the NodeMigrationAttribute.
|
@ikeough this looks good to me - is there anything else besides updating the API doc that needs to be done? |
Purpose
While developing a migration for the number node, a behavior was noticed in the migration framework that causes a second migration to not be applied when the first migration has the
tofield set tonull. Thetofield has never really been clear in its intent, and could potentially cause a migration to get "swallowed" by another migration. Ex. you have a migrationfrom: 0.0.0.1 to: 2.0.0.0and onefrom: 1.0.0.0 to: 2.0.0.0. The second migration would never be applied because the first would set the current version to2.0.0.0during the loop. Similarly, withoutto:defined as infrom: 0.0.0.1, any other migrations would not get processed because the loop exits whento:is null.This PR marks the
Tofield as obsolete onNodeMigrationAttribute, and adjusts the migration code to apply all migrations sequentially starting with the one whose version is>=the version of the stored workspace. All existing migrations have this intent, with migrations version ranges being sequential. So this change does not affect those migrations, and they will continue to be processed sequentially.This PR also migrates number nodes with sequences, i.e.
0..5to CBNs with0..5;. Currently, we do not allow the user to input0..5in a number node, but we do not disallow the number node from opening such a node. It has been tested to work on number nodes with numeric ranges like0..5and ranges with variables likea..b..c. All connectors are successfully re-established after migration.PTAL @mjkkirschner
FYI @gregmarr