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.

Sign up for GitHub

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

Jump to bottom

[DYN-3109] Disallow Python Migration Assistant when a Python3 engine is selected #11158

Merged
merged 23 commits into from
Oct 7, 2020
Merged

[DYN-3109] Disallow Python Migration Assistant when a Python3 engine is selected #11158

merged 23 commits into from
Oct 7, 2020

Conversation

zeusongit
Copy link
Contributor

@zeusongit zeusongit commented Sep 30, 2020 •
edited
Loading

Purpose

DYN-3109
Disallow Python Migration Assistant when a Python3 engine is selected
Screen Shot 2020-10-01 at 3 20 02 PM

Kapture 2020-09-30 at 16 20 26
(PS: I don't know why the gif is so slow, was normal before exporting)

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

Reviewers

@DynamoDS/dynamo

Zeus1717 added 13 commits April 5, 2020 12:46
@Zeus1717
DoesCurrentUserOwnPackage test update
8e56d0a
@Zeus1717
Merge remote-tracking branch 'upstream/master'
18bf789
@Zeus1717
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
d9fa3ee
@Zeus1717
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
c254158
@Zeus1717
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
dd8be20
@Zeus1717
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
e7b6018
@Zeus1717
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
f945dd7
@Zeus1717
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
a274b7d
@Zeus1717
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
8d6f97f
@Zeus1717
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
4edf5be
@Zeus1717
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
193655c
@Zeus1717
Disable migration button for cpython3 engine
1a6dfcd
@Zeus1717
Update ScriptEditorWindow.xaml
5764b46
QilongTang
QilongTang reviewed Oct 1, 2020
View reviewed changes
src/Libraries/PythonNodeModels/Properties/Resources.resx
@@ -192,7 +192,7 @@
<value>Use this dropdown to choose the python version/engine to execute your code.</value>
</data>
<data name="PythonScriptEditorMigrationAssistantButtonTooltip" xml:space="preserve">
<value>Migration Assistant to update your Python 2 scripts to Python 3.</value>
<value>Migration Assistant to update your Python 2 scripts to Python 3. Engine must be set to Python 2 version to work.</value>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Amoursol @aparajit-pratap Any more opinion on the messaging?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

QilongTang
QilongTang approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look solid to me, using converter seems a good choice here

@aparajit-pratap
Copy link
Contributor

@zeusongit could you check the test failures.

aparajit-pratap
aparajit-pratap reviewed Oct 1, 2020
View reviewed changes
Copy link
Contributor

@aparajit-pratap aparajit-pratap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is it true that without any hover effect, the only difference between the enabled and disabled state of the 2-3 button is that the tooltip appears and does not appear respectively?

@zeusongit
Copy link
Contributor Author

zeusongit commented Oct 1, 2020 •
edited
Loading

So is it true that without any hover effect, the only difference between the enabled and disabled state of the 2-3 button is that the tooltip appears and does not appear respectively?

There is also a slight difference in the border of the button.
Updated the description with a screenshot

@QilongTang
Copy link
Contributor

Running this branch on the BRE self CI to confirm the failures are real: https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/725/

mjkkirschner
mjkkirschner reviewed Oct 6, 2020
View reviewed changes
src/DynamoCoreWpf/DynamoCoreWpf.csproj Outdated
@@ -1248,6 +1248,10 @@
<Project>{6e0a079e-85f1-45a1-ad5b-9855e4344809}</Project>
<Name>Units</Name>
</ProjectReference>
<ProjectReference Include="..\Libraries\PythonNodeModels\PythonNodeModels.csproj">
<Project>{8872CA17-C10D-43B9-8393-5C5A57065EB0}</Project>
<Name>PythonNodeModels</Name>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm @zeusongit so now DynamoCoreWPF depends on PythonNodeModels, please add Private False to this entry to see if it helps the failing test issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also please consider if we really want DynamoCoreWPF to depend on PythonNodeModels, it seems like a strange reference.

QilongTang
QilongTang reviewed Oct 7, 2020
View reviewed changes
src/Libraries/PythonNodeModelsWpf/ScriptEditorWindow.xaml.cs Outdated
using System.Windows;
using System.Windows.Data;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need these references?

QilongTang
QilongTang reviewed Oct 7, 2020
View reviewed changes
src/Libraries/PythonNodeModelsWpf/ValueConverter.cs
using PythonNodeModels;
using System;
using System.Collections.Generic;
using System.Globalization;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question here. Are all of these used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes these are used, removed the other unused ones.

QilongTang
QilongTang approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a comment about using.

@zeusongit zeusongit merged commit 89fe860 into DynamoDS:master Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aparajit-pratap aparajit-pratap aparajit-pratap left review comments

@mjkkirschner mjkkirschner mjkkirschner left review comments

@QilongTang QilongTang QilongTang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zeusongit @aparajit-pratap @QilongTang @mjkkirschner @Zeus1717