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

Python template support 2.0 #8122

Merged
merged 23 commits into from Aug 28, 2017

Conversation

Projects
None yet
3 participants
@radumg
Contributor

radumg commented Aug 21, 2017

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 :
default python template

Behaviour when template file is found :
python custom example dynamo2.0

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)
    path
  • 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 #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

radumg added some commits Jul 11, 2017

added code regions and fixed annotations
- 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
- added public property to class for PythonTemplateFilePath
- added default value in the Settings constructor
added PythonTemplateFilePath to interfaces
Implemented support for the file path, following the model of the PreferencesFilePath.
read template for Python nodes + documentation
- 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
default Python template cleanup
- added an extra spacing line
- added spaces after `#` Python comment marker
add static backing store for PythonTemplateFIlePath
The backing store is required as a static property cannot implement an interface member.
add public static method to access private static prop
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.
replaced template get with static method call
gets rid of un-necessary initialisation of new `PreferenceSettings` or `PathManager` classes, see #8034 (review)

@radumg radumg changed the title from Python template to Python template support 2.0 Aug 21, 2017

@radumg radumg referenced this pull request Aug 21, 2017

Closed

Python template support #8034

7 of 7 tasks complete
changed PathManager python template prop to static
actually gets rid of un-necessary initialisation of new `PreferenceSettings` or `PathManager` classes, see #8034 (review)
forgot the PathManager in last commit.
@radumg

This comment has been minimized.

Show comment
Hide comment
@radumg

radumg Aug 21, 2017

Contributor

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.
dynamorevit

Contributor

radumg commented Aug 21, 2017

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.
dynamorevit

Show outdated Hide outdated .gitignore Outdated
@mjkkirschner

This comment has been minimized.

Show comment
Hide comment
@mjkkirschner

mjkkirschner Aug 24, 2017

Member

@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

Member

mjkkirschner commented Aug 24, 2017

@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

@radumg

This comment has been minimized.

Show comment
Hide comment
@radumg

radumg Aug 24, 2017

Contributor

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!

Contributor

radumg commented Aug 24, 2017

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!

radumg added some commits Aug 25, 2017

removed static PathManager props & added PyTemplate handling to Dynam…
…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`
#8122 (review)
@radumg

This comment has been minimized.

Show comment
Hide comment
@radumg

radumg Aug 25, 2017

Contributor

@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 🤓

Contributor

radumg commented Aug 25, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mjkkirschner

mjkkirschner Aug 25, 2017

Member

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

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.

Member

mjkkirschner commented Aug 25, 2017

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

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 settings files for tests
- 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`

radumg added some commits Aug 27, 2017

added unit test for CanUpdatePythonTemplateSettings
- 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
re-enable the temporarily disabled tests
these 2 tests were taking 10 minutes to run, had to disable them whilst building the new Python tests.
ensure order of execution is right
checking files required by test exist should be done before those files are read
@radumg

This comment has been minimized.

Show comment
Hide comment
@radumg

radumg Aug 27, 2017

Contributor

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

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

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.

Contributor

radumg commented Aug 27, 2017

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

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

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

This comment has been minimized.

Show comment
Hide comment
@mjkkirschner

mjkkirschner Aug 27, 2017

Member

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

Member

mjkkirschner commented Aug 27, 2017

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

@mjkkirschner

This comment has been minimized.

Show comment
Hide comment
@mjkkirschner

mjkkirschner Aug 28, 2017

Member

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

Member

mjkkirschner commented Aug 28, 2017

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

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

3 checks passed

EngOps Build EngOps Build Succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@radumg

This comment has been minimized.

Show comment
Hide comment
@radumg

radumg Aug 29, 2017

Contributor

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

Contributor

radumg commented Aug 29, 2017

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

@radumg radumg deleted the radumg:python-template branch Aug 29, 2017

@eibre

This comment has been minimized.

Show comment
Hide comment
@eibre

eibre Sep 6, 2017

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

eibre commented Sep 6, 2017

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

AngelaViVi added a commit to AngelaViVi/Expressior that referenced this pull request Oct 6, 2018

AngelaViVi added a commit to AngelaViVi/Expressior that referenced this pull request Oct 6, 2018

removed static PathManager props & added PyTemplate handling to Dynam…
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment