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

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

Merged
merged 4 commits into from
Mar 8, 2019

Conversation

alfarok
Copy link
Contributor

@alfarok alfarok commented Mar 1, 2019

Purpose

DYN-1507

This PR provides a strategy for a Dynamo host integrator to programmatically set the path of the Python nodes default template at startup. This is accomplished by setting the PythonTemplatePath on the configuration file that will be passed as a parameter to the DynamoModel when instantiated. It is required that the host integrator uses either the DefaultStartConfiguration struct or creates a struct that inherits from DefaultStartConfiguration. For example...

var config = new DynamoModel.DefaultStartConfiguration()
            {
                PythonTemplatePath = "C:\\Users\\alfarok\\Desktop\\PythonTemplate.py"
            };

var model = DynamoModel.Start(config);

Users are still able to override this file if desired by modifying the <PythonTemplateFilePath> attribute in the DynamoSettings.xml file or adding a a new template file in the appropriate AppData location (see below). To fallback to either host integrator path or the hard-coded template provided in Dynamo Core simply remove the path from the settings xml or delete the template file from the AppData location. Lastly, if a invalid path is provided the default Core template will be applied.

Logging

(In sequential order)

Loaded from DynamoSettings.xml

image

Loaded from Host Integrator

image

Loaded from AppData

image

Loaded Default Template

image

Overriding the default or a host integrator template as a USER

Users looking to override the template can do so in the following ways introduced in #8122:

Option 1 - Default AppData Location

Add a new python file named PythonTemplate.py to the C:\Users\alfarok\AppData\Roaming\Dynamo\Dynamo Core\2.2 directory

Option 2 - Custom Location using DynamoSettings.xml

Add a custom path to the DynamoSetting.xml under the PythonTemplateFilePath tag such as
<PythonTemplateFilePath>C:\Users\alfarok\Desktop\PythonTemplate.py</PythonTemplateFilePath>
(Does NOT require PythonTemplate.py naming)

Testing

Unit tests have been added to verify API usage for valid and invalid template paths. Additionally, this has been manually tested by modifying the DynamoSandbox startup config to include a PythonTemplatePath which is set here and invoked here. Testing coverage for loading the default template as well as a user template already has coverage.

Declarations

Check these if you believe they are true

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

Reviewers

@QilongTang @mjkkirschner

// To supply a custom python template host integrators should supply a 'DefaultStartConfiguration' config file
// or create a new struct that inherits from 'DefaultStartConfiguration' making sure to set the 'PythonTemplatePath'
// while passing the config to the 'DynamoModel' constructor.
var configCast = (DefaultStartConfiguration)config;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done because the DynamoModel constructor takes a IStartConfiguration config as its only param. I believe modifying the interface (even adding a new property) would be an API breaking change, but feel free to correct me if I am wrong on this or there is a better strategy.

Copy link
Member

Choose a reason for hiding this comment

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

I think you should do something like:
defaultConfig = config as DefaultStartConfiguration and then check for null because it's possible that the concreteType is NOT a DefaultStartConfiguration ... and that needs to be handled. (ie missing the python template field you just added)

Copy link
Member

Choose a reason for hiding this comment

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

alternatively you can add a second interface and check if the object implements that interface, if it does - do the right thing and set the path.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@mjkkirschner mjkkirschner Mar 4, 2019

Choose a reason for hiding this comment

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

in general, I don't think casting like this is a great idea for the problem we are discussing as casting can throw - as and is should not.

https://docs.microsoft.com/en-us/dotnet/api/system.invalidcastexception?view=netframework-4.7.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback guys. I was not happy with this approach either but as cannot be used with a non-nullable value type which DefaultStartConfiguration is as a struct. This is why I wrapped this logic in a try/catch returning the default template if an exception is thrown. However, performing a check such as

if(config is DefaultStartConfiguration)
{
    // Proceed in checking if a custom template path is available and valid
}
else
{
    // Use default template
}

is possible but does not guaranteed the PythonTemplatePath has been set. Attempting to retrieve the PythonTemplatePath property will simply return null if it was not set and I verify this is not the case below before proceeding. Long story short we can probably use is to verify the config is of the DefaultStartConfiguration type or of a type that inherits from it. I don't think adding another struct is necessary just for this property. If the config is of another type we return the default template. I believe the try/catch can also be removed with is as @mjkkirschner suggested since it will not throw. What do you guys think about this approach? @QilongTang

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

@QilongTang
Copy link
Contributor

@alfarok Once you wrap this up, please also attach the screen shot of console with this change so clients know what to expect from console. Thanks

@alfarok
Copy link
Contributor Author

alfarok commented Mar 6, 2019

@QilongTang I just added one new logging statement for when a custom template is loaded via the config, however it turns out none of the old statements ever successfully logged to the console or log file. Possibly because the DynamoModel is still being instantiated? I need to debug more but at first glad I don't see any exceptions thrown stepping through the logger

@QilongTang
Copy link
Contributor

@alfarok Sure, could be related to the sequence of how preference setting and logger are initialized

@alfarok
Copy link
Contributor Author

alfarok commented Mar 7, 2019

@QilongTang Added screenshots for all the logging statements. There were a couple issues with the previous logging logic which took me a little while to uncover. It appeared the logging wasn't working because the localized strings were embedded only in the Resources.resx file but should have also be in the Resources.en-US.resx, so logging was working but printing empty strings. I believe I enumerated all the scenarios and verified the correct statements align with the correct logic. Will get an some unit tests added and this should be good to go.

@alfarok alfarok changed the title [WIP] Provide a strategy for host integrator's to set the python template at startup Provide a strategy for host integrator's to set the python template at startup Mar 8, 2019
@alfarok
Copy link
Contributor Author

alfarok commented Mar 8, 2019

Added unit tests verifying API usage for valid and invalid template paths. I think this should be ready to go

@QilongTang
Copy link
Contributor

Look perfect on my side!

@alfarok alfarok added the LGTM Looks good to me label Mar 8, 2019
@alfarok alfarok merged commit 1686e2c into DynamoDS:master Mar 8, 2019
alfarok added a commit to alfarok/Dynamo that referenced this pull request Mar 8, 2019
…t startup (DynamoDS#9533)

* provide a strategy for host integrators to set Python node template at startup programmatically

* use 'is' as opposed to 'try/catch'

* Fix several bugs around previously existing logging statements

* add unit tests verifying valid and invalid  API usage
alfarok added a commit that referenced this pull request Mar 8, 2019
…t startup (#9533) (#9552)

* provide a strategy for host integrators to set Python node template at startup programmatically

* use 'is' as opposed to 'try/catch'

* Fix several bugs around previously existing logging statements

* add unit tests verifying valid and invalid  API usage
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.

3 participants