Conversation
| ImportAssembly(model, path); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
this makes it a lot easier (and less code) to use Dynamo for Design automation
There was a problem hiding this comment.
Ugh, you're right. I will refactor
| commonDataFolder: cmdLineArgs.CommonDataFolder, | ||
| serviceMode: cmdLineArgs.ServiceMode); | ||
| serviceMode: cmdLineArgs.ServiceMode, | ||
| importedPaths: cmdLineArgs.ImportedPaths); |
There was a problem hiding this comment.
I'm confused why this is necessary?
the CLI already sets imported paths from cli args, doesn't it?
https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoApplications/StartupUtils.cs#L100
| /// </summary> | ||
| /// <param name="model"></param> | ||
| /// <param name="path"></param> | ||
| internal static void ImportAssembly(DynamoModel model, string path) |
There was a problem hiding this comment.
any thoughts on how to unify these duplicate functions now?
https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCLI/CommandLineRunner.cs#L96
I don't know why the DynamoCoreWPFCLI did not use the protected method, does it not inherit from the base class?
There was a problem hiding this comment.
I refactored the methods to use a single import function.
|
@pinzart90 for posterity can you either change the description or title of this PR to be more clear that you want this functionality for the DynamoModel in general? To me anyway it was confusing at first glance because this functionality already exists for the CLIs. |
pinzart90
changed the title
Sorry about the confusion. I want the import functionality to be available for the |
| commonDataFolder: cmdLineArgs.CommonDataFolder, | ||
| serviceMode: cmdLineArgs.ServiceMode); | ||
|
|
||
| ImportAssemblies(model, cmdLineArgs.ImportedPaths); |
There was a problem hiding this comment.
Is there an extra import step happening here that wasn't happening before?
There was a problem hiding this comment.
This definitely sticks out as a strange place for this code to live. I would just expect this method to create a dynamo model?
There was a problem hiding this comment.
It seems like you are trying to get around your implementation in that you're creating a CLI model in design automation but you're not really using a CLI entry point?
There was a problem hiding this comment.
Well, why should it matter if the model is used by DynamoCLI or another CLI (in my case the DesignAutomationCLI).
My indention is to have a simple, clean way to build a model for the purposes of being used in a headless way
There was a problem hiding this comment.
The cmdLineArgs input (for MakeCLIModel) allows specifying ImportedPaths, but it is not used.
There is no public API to import libs (that I could find). I could call the import API outside of the MakeModel call/..with no need for this PR
There was a problem hiding this comment.
Is this clean?
This function doesn't do what it says on the tin anymore.
Won't this also cause the dynamo CLIs to attempt to import these assemblies twice- I guess that's probably, ok - though it could be slow?
why can't DesignAutomationCLI use the same set of flags the other CLI's do and just import assemblies that way?
There was a problem hiding this comment.
sorry, I did not notice the double import call
There was a problem hiding this comment.
evidently not simple or clear enough
I can simply try to expose the Import function...or the existing StartupDynamo method, and revert everythign else
There was a problem hiding this comment.
Part of the confusion is that the method MakeCLIModel has arguments that take no effect when you make the function call. Maybe it should be made clear that the importPaths are not meant for that function ...
3 participants
Purpose
Add import libs to the
MakeCLIModel(CommandLineArguments cmdLineArgs)APIDeclarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of