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

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

Merged
merged 4 commits into from
Mar 8, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c8bb1a
provide a strategy for host integrators to set Python node template a…
alfarok Mar 1, 2019
301aeb1
use 'is' as opposed to 'try/catch'
alfarok Mar 6, 2019
7681b36
Fix several bugs around previously existing logging statements
alfarok Mar 7, 2019
96aba95
add unit tests verifying valid and invalid API usage
alfarok Mar 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 48 additions & 7 deletions src/DynamoCore/Models/DynamoModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,7 @@ public struct DefaultStartConfiguration : IStartConfiguration
public IEnumerable<IExtension> Extensions { get; set; }
public TaskProcessMode ProcessMode { get; set; }
public bool IsHeadless { get; set; }
public string PythonTemplatePath { get; set; }
}

/// <summary>
Expand Down Expand Up @@ -640,17 +641,42 @@ protected DynamoModel(IStartConfiguration config)
// If not, check the default filepath for the Python template (userDataFolder\PythonTemplate.py).
// If that doesn't exist either or is empty, Dynamo falls back to the hard-coded Python script template.
// We log the behaviour to make it easy for users to troubleshoot this.
if (string.IsNullOrEmpty(PreferenceSettings.PythonTemplateFilePath) || !File.Exists(PreferenceSettings.PythonTemplateFilePath))

// If a custom python template path doesn't already exists in the DynamoSettings.xml
if(string.IsNullOrEmpty(PreferenceSettings.PythonTemplateFilePath) || !File.Exists(PreferenceSettings.PythonTemplateFilePath))
{
if (string.IsNullOrEmpty(pathManager.PythonTemplateFilePath) || !File.Exists(pathManager.PythonTemplateFilePath))
Logger.Log(Resources.PythonTemplateInvalid);
else
try
{
// 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.

👍 Same options in https://jira.autodesk.com/browse/DYN-1644

Copy link
Member

@mjkkirschner mjkkirschner Mar 4, 2019 •
edited
Loading

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.

var templatePath = configCast.PythonTemplatePath;

// If a custom python template path was set in the config apply that template
if (!string.IsNullOrEmpty(templatePath) && File.Exists(templatePath))
{
PreferenceSettings.PythonTemplateFilePath = templatePath;
Logger.Log(Resources.PythonTemplateDefinedByHost + " : " + pathManager.PythonTemplateFilePath);
}

// Otherwise fallback to the default
else
{
SetDefaultPythonTemplate();
}
}
catch
{
PreferenceSettings.PythonTemplateFilePath = pathManager.PythonTemplateFilePath;
Logger.Log(Resources.PythonTemplateDefaultFile + " : " + pathManager.PythonTemplateFilePath);
// Fallback to the default
SetDefaultPythonTemplate();
}
}
else Logger.Log(Resources.PythonTemplateUserFile + " : " + pathManager.PythonTemplateFilePath);
else
{
// A custom python template path already exists in the DynamoSettings.xml
Logger.Log(Resources.PythonTemplateUserFile + " : " + pathManager.PythonTemplateFilePath);
}

pathManager.Preferences = PreferenceSettings;

Expand Down Expand Up @@ -779,6 +805,21 @@ protected DynamoModel(IStartConfiguration config)
DynamoReady(new ReadyParams(this));
}

private void SetDefaultPythonTemplate()
{
// Check if default python template file path is valid, if so apply template
if (!string.IsNullOrEmpty(pathManager.PythonTemplateFilePath) && File.Exists(pathManager.PythonTemplateFilePath))
{
PreferenceSettings.PythonTemplateFilePath = pathManager.PythonTemplateFilePath;
Logger.Log(Resources.PythonTemplateDefaultFile + " : " + pathManager.PythonTemplateFilePath);
}

else
{
Logger.Log(Resources.PythonTemplateInvalid);
}
}

private void DynamoReadyExtensionHandler(ReadyParams readyParams, IEnumerable<IExtension> extensions) {

foreach (var ext in extensions)
Expand Down
13 changes: 11 additions & 2 deletions src/DynamoCore/Properties/Resources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion src/DynamoCore/Properties/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,7 @@ value : bool = false</value>
<value>Python template set to default file</value>
</data>
<data name="PythonTemplateUserFile" xml:space="preserve">
<value>Python template set to user file</value>
<value>Python template loaded from DynamoSettings.xml path</value>
</data>
<data name="DefaultHomeWorkspaceName" xml:space="preserve">
<value>Home</value>
Expand Down Expand Up @@ -697,4 +697,7 @@ The input name should be a valid variable name, without spaces. An input type an
<data name="InvalidInputSymbolWarningTitle" xml:space="preserve">
<value>Custom Node Contains Invalid Inputs and Cannot Be Saved.</value>
</data>
<data name="PythonTemplateDefinedByHost" xml:space="preserve">
<value>Python template set by host integrator</value>
</data>
</root>