Python template support 2.0 #8122

radumg · 2017-08-21T22:44:59Z

Note : this PR replaces the previous PR #8034 as that was based on 1.3.1 branch and not on master. Re-basing was messy, so cherry-picked to this new PR instead.

Purpose

This PR fixes #7604 , wish for adding a Python template that can be used to populate any Python Script nodes when adding to workspace.

Current behaviour

user adds Python Script node to canvas
the node is populated with a few lines of code, but these are hard-coded in the Dynamo source code, so users are unable to change them.

New behaviour

user adds Python Script node to canvas
Dynamo checks if the PythonTemplate.py file exists at the user location root (%appdata%/Dynamo/Core/{version}/) and if it does, it reads the file and populates the Python script node with its contents
if the file doesn't exits or it's empty, Dynamo falls back on the hard-coded behaviour

Behaviour when template file is missing or empty :

Behaviour when template file is found :

This allows users and organisations to rollout the template as part of users profiles or as logon scripts.

Implementation

refactored PreferenceSettings class to include code regions, moving properties into their logical group, also fixing some comments/documentation at same time, making it clearer.
implemented a PythonTemplateFilePath property in the settings class so it serialises and deserialises with user settings
implemented required properties in the interfaces IPreferences, IPathResolver and PathManager class
implemented a static backing store called pythonTemplateFilePath. This is what the property above (part of interface) gets and sets. This was necessary as static props cannot be part of the interface.
exposed the private static property with a public static method called GetPythonTemplateFilePath, so it's accessible by nodes and anything else running in Dynamo context, such as ZT nodes.
added a few additional lines to the hardcoded python template, providing pointers to users where to start writing code and explaining the first few lines (clr imports, etc). This is following my observations in-person and on forums that most new users struggle with those 2 things : where to put code and what those bits mean.
replaced \n in hardcoded python code template with .NET standard Environment.NewLine line breaks, should behave more predictably when compiled on different platforms
replaced hardcoded comments in python code with texts extracted into .resx variables, to add support for localisation of code comments too.
got rid of un-necessary initialisation of new PreferenceSettings or PathManager classes as per @mjkkirschner 's review. What happens now is that everytime a new DynamoModel instance is created, the backing static property is overwritten.

Limitations

the name and location of template file is hardcoded. however, this is consistent/same as DynamoSettings.XML file itself. Since the interfaces implement the property, it can be exposed via UI in later PR (working on this already but I'm not great with WPF, could use a helping hand)
I couldn't figure out how to access the current DynamoModel instance from the PythonNodeModel class to retrieve current settings, hence the static property, as discussed in previous PR Python template support #8034
the template file is read every time a Python Script node is added to canvas, there's no caching - can add it if required ?
As a bonus though, the above allows for quick iterations of the template in external editor :)
the public static method technically duplicates the private static property, but I felt this was cleaner than exposing 2 properties with almost same name with just different capitalisation and one being static

Declarations

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.
Based on master branch with 2.0 reported version

Reviewers

@kronz
@mjkkirschner
@smangarole
@QilongTang

FYIs

@ksobon , @ThomasMahon, @wynged , @eibre , @andydandy74

- introduced code regions - moved class properties to appropriate regions - fixed a few summary annotations to make it clearer what they do/return and adhere to Microsoft standards a bit better

- added public property to class for PythonTemplateFilePath - added default value in the Settings constructor

Implemented support for the file path, following the model of the PreferencesFilePath.

- when adding a Python node, the template is read - added conditional load & fallback on hardcoded text if template is not found - replaced "\n" with `Environment.NewLine` in hardcoded template - added new comment lines to default template, using Resources to make it easier for beginners - fixed typo in existing Resources & XAML

- added an extra spacing line - added spaces after `#` Python comment marker

The backing store is required as a static property cannot implement an interface member.

The backing store is private & static, so it doesn't get serialised when settings are saved. Hence, we add a public static method to access it.

gets rid of un-necessary initialisation of new `PreferenceSettings` or `PathManager` classes, see DynamoDS#8034 (review)

actually gets rid of un-necessary initialisation of new `PreferenceSettings` or `PathManager` classes, see DynamoDS#8034 (review) forgot the PathManager in last commit.

radumg · 2017-08-21T23:47:24Z

FYI @mjkkirschner , checked for dependency on changed DynamoCore interfaces in the DynamoRevit repo and they should be fine.
The file you referenced depends on IPathResolver which was not changed, only IPathManager was updated.

mjkkirschner · 2017-08-22T01:28:13Z

.gitignore

 test/core/customast/test.txt
 test/core/files/test.png
 test/core/migration/writetext.txt
+src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs


I'm not sure, but I believe there is a reason we don't have this as part of the gitignore, you can simply not commit changes to this file until we dig deeper.

Thanks @mjkkirschner , looked around and it was already part of the .gitignore, see a few lines above - my GitHub desktop app added it again when i ignored it. I untracked the AssemblySharedInfo.cs file in my local repo now, just need to do the same for this .gitignore file too.

fixed it now 👍

removed duplicate file tracking for `AssemblySharedInfo.cs` https://github.com/DynamoDS/Dynamo/blob/8e8fe12f89150be1fe7f24edf9931859b59c731a/.gitignore#L74

mjkkirschner · 2017-08-24T22:36:28Z

src/DynamoCore/Configuration/PathManager.cs

            yield return Path.GetFullPath(library);
        }

+        internal static string GetPythonTemplatePath()


@radumg thanks for looking into how to avoid the creation of a new pathManager, but this smells funny to me, and it is a bit inconsistent with other preferences which are similar, like the customPackageFolders paths.

I'm thinking we could follow a similar pattern like here:

Dynamo/src/DynamoCore/Models/DynamoModel.cs

Line 583 in 7304ba1

if (PreferenceSettings.CustomPackageFolders.Count == 0)

so we would assign the default path to "" in the preferences constructor and instead set it to path specified in the pathManager during DynamoModel creation time.

Let me know what you think of this - both seem okay on their own, but this seems a more consistent solution and we can avoid the static method on pathManager

Thanks @mjkkirschner - think your suggestion is good, it's indeed inconsistent with rest of props. I'll go ahead and implement the above, was just trying to avoid modifying the DynamoModel class/ctor as per our previous conversations.

Ok, I've implemented this in c4bff15
I added some logic so it follows this flow :

tries to use template file specified in DynamoSettings.xml first

if that fails, it looks for the default PythonTemplate.py in the user folder. If this is found, the setting will be updated to point to it.

if both those fail, it will fall back on the hardcoded string.

Also added logging so if users are having a hard time getting their templates to be picked up (most likely due to invalid paths), they would have some indication of what's going on.

Let me know if this is what you had in mind and if there's any changes you'd like.

mjkkirschner · 2017-08-24T22:44:36Z

src/DynamoCore/Configuration/PreferenceSettings.cs

+        }
+
+        /// <summary>
+        /// Returns the static Python template file path


since this is the public entry point for most use cases lets add some description of what this file at this path is used for.

Good point, wasn't being descriptive enough 👍 . Added now, have a look please 👁

mjkkirschner · 2017-08-24T22:45:49Z

src/Libraries/PythonNodeModels/PythonNodeModels.csproj

    </Reference>
    <Reference Include="System" />
    <Reference Include="System.Core" />
+    <Reference Include="System.Windows.Forms" />


is this reference still needed for something?

nope, forgot to remove it - I had a MessageBox.Show() during testing. Will keep an eye out for these going forward.
It's now removed.

mjkkirschner · 2017-08-24T22:51:06Z

@radumg thanks for the work here. I notice there are no new tests. Would it be possible to add a test for this preference, like:

https://github.com/DynamoDS/Dynamo/blob/dec6240ded0c4369617775336b9af60c2aba4103/test/DynamoCoreTests/Settings.cs

which just asserts the deserialization is working correctly

and then something which modifies the template then asserts the python script changes? A potential test location is here:

https://github.com/DynamoDS/Dynamo/blob/master/test/Libraries/DynamoPythonTests/PythonEditTests.cs

…to python-template

DynamoDS#8122 (comment)

radumg · 2017-08-24T23:57:34Z

Thanks @mjkkirschner for the review. I thought about potential tests for a second but couldn't imagine any useful ones - obvs my imagination failed me 😁 - I can certainly try & add the tests you suggest above.

I'm fairly new to TDD so I really appreciate the guidance!

…oModel - removed static getter in PathManager and changed backing prop to instance - added handling of user Python file, default file or no template - added logging - added log messages to `*.resx` DynamoDS#8122 (review)

radumg · 2017-08-25T15:43:28Z

@mjkkirschner - if you happen to see this before i start working on it again tonight, i'm having a hard time adding any Dynamo node to the Workspace for this last test. Tried using the same base class and Workspace.AddAndRegisterNode method as here but that method is not accessible from the IronPythonTests assembly. Can't find a replacement mechanism to add a node to current workspace from this context.

Just thought I'd check before I delve deeper, pretty sure I'm missing an obvious trick here 🤓

mjkkirschner · 2017-08-25T16:49:52Z

@radumg what you can do is use a createNode command and execute it on the dynamoModel

Dynamo/test/DynamoCoreTests/NodeExecutionTest.cs

Line 58 in dec6240

    
           model.ExecuteCommand(new DynamoModel.CreateNodeCommand(numberNode1, 0, 0, true, false));

Another possibility is to make the python test assembly able to access internal members of other assemblies with
https://msdn.microsoft.com/en-us/library/system.runtime.compilerservices.internalsvisibletoattribute(v=vs.110).aspx - we use this for tests though it's not ideal.

- added XML settings files for tests - renamed existing settings file to fit in better with structure (easier to group & read by humans) - consolidated files for previous test `oadInvalidPythonTemplateFromSetting` in `Settings.cs`

- test passes locally - test requires 2 new python files, included them in same folder - test generates 1 additional settings file when run, `DynamoSettings-PythonTemplate-changed.XML` - test does not depend on hard-coded paths

these 2 tests were taking 10 minutes to run, had to disable them whilst building the new Python tests.

checking files required by test exist should be done before those files are read

radumg · 2017-08-27T20:47:38Z

Ok, I think I've figured out these tests 🎉 & 🤞

I've added 3 tests in total @mjkkirschner (screenshot only shows 2) :

Test 1 : LoadInvalidPythonTemplateFromSetting in test/DynamoCoreTests/Settings.cs#39

loads the settings from XML
checks the settings were deserialised correctly
checks the file with invalid path doesn't exist

Test 2 : CanReadPythonTemplateSetting in test/DynamoCoreTests/CoreTests.cs#55

same as test 1 but in this different execution context

Test 3 : CanUpdatePythonTemplateSettings in test/DynamoCoreTests/CoreTests.cs#77

loads the settings from XML
checks the settings were deserialised correctly
updates the DynamoModel with settings
creates & adds a PythonNode to workspace
checks added node has been initialised with the custom initial Python template
changes the Python template
reloads the settings from the updated XML & checks the settings were deserialised correctly again
updates the DynamoModel with settings again
creates & adds another PythonNode to workspace
checks added node has been initialised with the custom changed Python template

It's ready for review again @mjkkirschner , thanks so much for the pointers !
Let me know if there's any more changes needed.

mjkkirschner · 2017-08-27T23:47:07Z

@radumg this looks solid to me, will take another look and bring it in soon.

mjkkirschner · 2017-08-28T19:58:56Z

@QilongTang tests pass- going to merge this in, lets watch the revit build.

radumg · 2017-08-29T20:50:41Z

Awesome, just wanted to say thanks again @mjkkirschner for the guidance along the way!
On to DYN template support now... 🤓

eibre · 2017-09-06T08:26:18Z

Thank you so much for this @radumg , it's working great.

DynamoDS/Dynamo#8122 (comment)

…oModel - removed static getter in PathManager and changed backing prop to instance - added handling of user Python file, default file or no template - added logging - added log messages to `*.resx` DynamoDS/Dynamo#8122 (review)

MarkAckerley · 2019-04-25T11:03:24Z

Thanks @radumg it's working on a network path :)

Just an FYI, when I follow the instruction in the primer, if I reopen the DynamoSettings.xml it has changed:

<PythonTemplateFilePath>
<string>Z:\DYNAMO\PythonTemplate.py</string>
</PythonTemplateFilePath>

to:

  <CustomPackageFolders>
    <string>Z:\DYNAMO\PythonTemplate.py</string>
    <string>C:\Users\mka\AppData\Roaming\Dynamo\Dynamo Revit\2.0</string>
  </CustomPackageFolders>
  <PackageDirectoriesToUninstall />
  <PythonTemplateFilePath>Z:\DYNAMO\PythonTemplate.py</PythonTemplateFilePath>
  <BackupInterval>60000</BackupInterval>

Thanks again,

Mark

radumg added 10 commits August 17, 2017 22:40

added code regions and fixed annotations

d99134f

- introduced code regions - moved class properties to appropriate regions - fixed a few summary annotations to make it clearer what they do/return and adhere to Microsoft standards a bit better

added PythonTemplateFile setting

7eb91cc

- added public property to class for PythonTemplateFilePath - added default value in the Settings constructor

added PythonTemplateFilePath to interfaces

8e9c755

Implemented support for the file path, following the model of the PreferencesFilePath.

default Python template cleanup

a12d5df

- added an extra spacing line - added spaces after `#` Python comment marker

add static backing store for PythonTemplateFIlePath

c13ef1c

The backing store is required as a static property cannot implement an interface member.

moved hardcoded Python template to readonly prop

ad36dae

add public static method to access private static prop

bd658e4

The backing store is private & static, so it doesn't get serialised when settings are saved. Hence, we add a public static method to access it.

fix Python template un-commented line

773a416

replaced template get with static method call

2f234bb

gets rid of un-necessary initialisation of new `PreferenceSettings` or `PathManager` classes, see DynamoDS#8034 (review)

radumg changed the title ~~Python template~~ Python template support 2.0 Aug 21, 2017

radumg mentioned this pull request Aug 21, 2017

Python template support #8034

Closed

7 tasks

changed PathManager python template prop to static

9308735

actually gets rid of un-necessary initialisation of new `PreferenceSettings` or `PathManager` classes, see DynamoDS#8034 (review) forgot the PathManager in last commit.

mjkkirschner reviewed Aug 22, 2017

View reviewed changes

Update .gitignore

d221139

removed duplicate file tracking for `AssemblySharedInfo.cs` https://github.com/DynamoDS/Dynamo/blob/8e8fe12f89150be1fe7f24edf9931859b59c731a/.gitignore#L74

mjkkirschner reviewed Aug 24, 2017

View reviewed changes

radumg added 3 commits August 24, 2017 23:53

remove unnecessary reference

5be6f8b

Merge branch 'python-template' of https://github.com/radumg/Dynamo in…

6227c43

…to python-template

add description for static method

2319431

DynamoDS#8122 (comment)

radumg added 2 commits August 25, 2017 02:26

added PythonTemplate file test

a99bde1

radumg added 2 commits August 27, 2017 18:03

added settings files for tests

9ff3479

- added XML settings files for tests - renamed existing settings file to fit in better with structure (easier to group & read by humans) - consolidated files for previous test `oadInvalidPythonTemplateFromSetting` in `Settings.cs`

added unit test for CanUpdatePythonTemplateSettings

bdb6c23

- test passes locally - test requires 2 new python files, included them in same folder - test generates 1 additional settings file when run, `DynamoSettings-PythonTemplate-changed.XML` - test does not depend on hard-coded paths

radumg added 4 commits August 27, 2017 21:08

re-enable the temporarily disabled tests

5a73bf0

these 2 tests were taking 10 minutes to run, had to disable them whilst building the new Python tests.

update Settings tests with Python test

239fd8c

ensure order of execution is right

083fce5

checking files required by test exist should be done before those files are read

small consistency fixes to tests

0ba2bc8

mjkkirschner merged commit 6127470 into DynamoDS:master Aug 28, 2017

radumg deleted the python-template branch August 29, 2017 20:50

jiafeng5513 pushed a commit to jiafeng5513/Expressior that referenced this pull request Oct 6, 2018

add description for static method

17cb8b2

DynamoDS/Dynamo#8122 (comment)

alfarok mentioned this pull request Mar 1, 2019

Provide a strategy for host integrator's to set the python template at startup #9533

Merged

7 tasks

Python template support 2.0 #8122

Python template support 2.0 #8122

Uh oh!

Conversation

radumg commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Current behaviour

New behaviour

Implementation

Limitations

Declarations

Reviewers

FYIs

Uh oh!

radumg commented Aug 21, 2017

Uh oh!

mjkkirschner Aug 22, 2017

Choose a reason for hiding this comment

Uh oh!

radumg Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radumg Aug 22, 2017

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radumg Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

radumg Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

radumg Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

radumg Aug 24, 2017

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

radumg commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

radumg commented Aug 25, 2017

Uh oh!

mjkkirschner commented Aug 25, 2017

Uh oh!

radumg commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Aug 27, 2017

Uh oh!

mjkkirschner commented Aug 28, 2017

Uh oh!

radumg commented Aug 29, 2017

Uh oh!

eibre commented Sep 6, 2017

Uh oh!

MarkAckerley commented Apr 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

radumg commented Aug 21, 2017 •

edited

Loading

radumg Aug 22, 2017 •

edited

Loading

mjkkirschner Aug 24, 2017 •

edited

Loading

radumg Aug 25, 2017 •

edited

Loading

mjkkirschner commented Aug 24, 2017 •

edited

Loading

radumg commented Aug 24, 2017 •

edited

Loading

radumg commented Aug 27, 2017 •

edited

Loading