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

[DYN-2322] Add PythonNet 3.7 CPython as a selectable engine. #10548

Merged
merged 18 commits into from
Apr 21, 2020

Conversation

reddyashish
Copy link
Contributor

@reddyashish reddyashish commented Apr 7, 2020

Purpose

This PR is a cloned from the original PR(#10472) by @Dewb.

The auto-completion part for the IronPython is fixed. The IronPythonCompletionData and IronPythonCompletionProvider classes have been moved from the PythonNodeModelsWPF project to the DSIronPython project. We can extend this similarly with the new DSCPython.

There is only one failing test left that is related to function marshaling: FirstClassFunctions

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

@DynamoDS/dynamo

@reddyashish reddyashish added the WIP label Apr 7, 2020
src/Dynamo.All.sln Outdated Show resolved Hide resolved
@reddyashish reddyashish changed the base branch from master to DSCPython3 April 16, 2020 18:47
@reddyashish reddyashish changed the title [WIP] [DYN-2322] Add PythonNet 3.7 CPython as a selectable engine. [DYN-2322] Add PythonNet 3.7 CPython as a selectable engine. Apr 17, 2020
@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 17, 2020 via email

@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 17, 2020 via email

@reddyashish
Copy link
Contributor Author

@mjkkirschner Yes, will add them back.

</ItemGroup>
<ItemGroup>
<EmbeddedResource Include="Resources\property.png" />
</ItemGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason for moving things from PythonNodeModelsWpf to here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These property are being used for the auto-completion feature and we have moved all the auto-completion code to the DSIronPython project. The idea was not to have this PythonNodeModelsWPF project directly depend on any python binaries.

</assembly>
<namespaces>
<namespace name="DSIronPython">
<category>Core.Scripting</category>
Copy link
Member

@mjkkirschner mjkkirschner Apr 17, 2020

Choose a reason for hiding this comment

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

@reddyashish why is this being deleted? This will effect where nodes go in the library won't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where this xml is used but the python node is being saved in the Script -> Editor folder in the library section(same as before).

src/Libraries/DSCPython/CPythonEvaluator.cs Outdated Show resolved Hide resolved
src/Libraries/DSCPython/CPythonEvaluator.cs Outdated Show resolved Hide resolved
src/Libraries/DSCPython/CPythonEvaluator.cs Show resolved Hide resolved
src/Libraries/PythonNodeModels/PythonNode.cs Outdated Show resolved Hide resolved
src/Libraries/PythonNodeModels/PythonNode.cs Outdated Show resolved Hide resolved
@Dewb Dewb mentioned this pull request Apr 20, 2020
6 tasks
@QilongTang
Copy link
Contributor

QilongTang commented Apr 20, 2020

Overall looks good to me and fairly safe to merge into the feature branch for our next moves.

  1. I saw there is a binary removed DynamoPython, so we may want to figure out a way to create empty binary for that this year because of the limitation of patch installer.
  2. We can ask BRE to build this branch in the newly setup VMs to see if builds. Seems those VMs already have .NetFramework 4.7.2 installed because it is OOTB with VS2019

@aparajit-pratap @mjkkirschner Any other notes?

…t being used.

These binaries are added as reference to the DSIronPython project and the private flag is set to true. So the binaries are being copied to the bin folder.
<Reference Include="ICSharpCode.AvalonEdit">
<HintPath>..\..\..\extern\avalonEdit\ICSharpCode.AvalonEdit.dll</HintPath>
<Private>False</Private>
</Reference>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjkkirschner The IronPython SQL, Modules and WPF binaries are already added as references here to the DSIronPython project and the private flag is set to true. So the binaries are copied to the bin folder.

@reddyashish
Copy link
Contributor Author

@QilongTang Yes, I will add an empty project in place of DynamoPython.

@QilongTang
Copy link
Contributor

@reddyashish Looks good, can we merge this now?

@QilongTang
Copy link
Contributor

QilongTang commented Apr 21, 2020

@DynamoDS/dynamo I am merging this so that BRE can test branch builds on new VM..

@QilongTang QilongTang merged commit c853363 into DynamoDS:DSCPython3 Apr 21, 2020
<ProjectReference Include="..\DSIronPython\DSIronPython.csproj">
<Project>{9eef4f42-6b3b-4358-9a8a-c2701539a822}</Project>
<Name>DSIronPython</Name>
<Private>False</Private>
Copy link
Member

Choose a reason for hiding this comment

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

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.

6 participants