T1.1 Python 2 Code Migration

[SHKnudsen]

Purpose

This PR is the first step in adding a Migration Assistant that helps users automatically upgrade their Python code from Python 2 to Python 3. The PR focuses on adding the functionality to Migrate Python 2 code to Python 3 compatible code, using Pythons 2to3 library. It also adds a new VisualDifferenceViewer to show the changes being made by the migration.

This is the biggest of the PRs concerning the Python 3 migration work, the following PRs will be smaller.

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.

Reviewers

@mjkkirschner
@QilongTang
@radumg

FYIs

The VisualDifferenceViewer are using the DiffPlex library. I know the decision on whether or not we want to include this dependency is still pending, but the rest of the code should be fine to review.

The button icons on the VisualDifferenceViewer are also pending final confirmation, we can easily revert back to text if this turns out to be an issue.

[QilongTang]

Thank you @SHKnudsen . This is exciting. @mjkkirschner will log a Jira task for our team to review your work soon

[mjkkirschner]

https://jira.autodesk.com/browse/DYN-2888

[mmisol]

Just to move things up a little, I commented the things I feel still need addressing. I'll summarize here:

• Delete third unit test (unless there is something I overlooked, LMK)
• Add new libraries to license files
• Fix bug related to unsaved code changes not showing in the migrator

[mjkkirschner]

Thanks for the reminder Martin - as it stands I would appreciate info on loading fixers if that’s something that was considered already, but if not I don’t want to hold up this pr for comments on a feature we may not use.

…

On Jul 20, 2020, at 8:56 AM, Martin Misol Monzo ***@***.***> wrote: @mmisol commented on this pull request. Just to move things up a little, I commented the things I feel still need addressing. I'll summarize here: Delete third unit test (unless there is something I overlooked, LMK) Add new libraries to license files Fix bug related to unsaved code changes not showing in the migrator In test/DynamoCoreTests/PythonMigrationAssistantTests.cs: > + + [Test] + public void MigrationWillNotChangeCodeAfterBeingConverted() + { + // Arrange + var originalPython2Code = @"for x in xrange(100): continue"; + var expectedPython3CodeAfterMigration = @"for x in range(100): continue"; + + // Act + var migratedScript = ScriptMigrator.MigrateCode(originalPython2Code); + var migratedScript2 = ScriptMigrator.MigrateCode(migratedScript); + + // Assert + Assert.AreEqual(expectedPython3CodeAfterMigration, migratedScript); + Assert.AreEqual(expectedPython3CodeAfterMigration, migratedScript2); + } @radumg Since you mentioned idempotency let's get mathematical :) The second test shows that x = f(x) when x doesn't have Python 2 in it. The third tests shows that y = f(f(y)) with a y with the same characteristics as x in the previous test. The second test is already showing idempotency, therefore my argument to delete this third test. In test/DynamoCoreTests/PythonMigrationAssistantTests.cs: > + [Test] + public void MigrationWillNotChangeCodeAfterBeingConverted() + { + // Arrange + var originalPython2Code = @"for x in xrange(100): continue"; + var expectedPython3CodeAfterMigration = @"for x in range(100): continue"; + + // Act + var migratedScript = ScriptMigrator.MigrateCode(originalPython2Code); + var migratedScript2 = ScriptMigrator.MigrateCode(migratedScript); + + // Assert + Assert.AreEqual(expectedPython3CodeAfterMigration, migratedScript); + Assert.AreEqual(expectedPython3CodeAfterMigration, migratedScript2); + } + } Brought this to standup the other day and I didn't get any remarks from Mike. Unless he comments otherwise, let's assume we don't need this for now, for the sake of unblocking this PR. In src/PythonMigrationViewExtension/packages.config: > @@ -0,0 +1,7 @@ +<?xml version="1.0" encoding="utf-8"?> +<packages> + <package id="DiffPlex" version="1.6.3" targetFramework="net48" /> + <package id="DiffPlex.Wpf" version="1.1.1" targetFramework="net48" /> @SHKnudsen Please address this. In src/Libraries/PythonNodeModelsWpf/ScriptEditorWindow.xaml.cs: > @@ -167,6 +167,13 @@ private void OnRunClicked(object sender, RoutedEventArgs e) } } + private void OnMigrationAssistantClicked(object sender, RoutedEventArgs e) + { + if (nodeModel == null) + throw new NullReferenceException(nameof(nodeModel)); + + nodeModel.RequestCodeMigration(e); + } Please address this bug before merging @SHKnudsen — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[SHKnudsen]

@mmisol addressed the last changes, sorry for the delay! Let me know if there is anything else.

[mmisol]

Thanks @SHKnudsen . I see the bug is fixed. Could I ask you to place diffplex in the license.rtf file so that it appears in alphabetical order in the about window?

[SHKnudsen]

@mmisol order of the licence files are updated

[mmisol]

Thanks @SHKnudsen! Looks good to me

[mmisol]

I couldn't reproduce the test failure locally. I have triggered a manual test run on a different server just in case this is just a flaky failure. If that works I'm merging this.

[mmisol]

Also got an error here https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/709/, although it seems unrelated (PackageDependencyTests). I think we can probably merge but I'll confirm with @QilongTang first.