node docs generator tool without MetaDataLoadContext by mjkkirschner · Pull Request #11950 · DynamoDS/Dynamo

mjkkirschner · 2021-08-19T20:15:59Z

Purpose

Please see @SHKnudsen's PR for a great summary:
#11725

differences between the PRs:

MetaDataLoadContext support has been removed for the current time, and we'll explore its necessity when we deploy this tool. This removes ~ 700 LOC.
- This means that in general full dependencies are required (ie all managed Revit API dlls)
- As a consequence the tool now generates 100 additional nodes it was missing before - see the readme for details.
removes MetaDataLoadContext nuget packages.
animated gif compression has been added. -g
adds verbose -v mode- I found helpful to track down some issues.
adds ImageMagick .net and native to the license and about box.
adds a readme - mostly stolen from @SHKnudsen's summary, but also with some of our use cases described - hopefully to help devs understand what aspects are important to maintain.
moves layout spec with a type forward attribute so the exe no longer depends on DynamoCoreWPF.

TODO:

after this is merged, I will file the known issues as tasks / spikes for us to pick up later.
after this is merged I will work with Sylvester on this PR: Node markdown generator tool unit tests SHKnudsen/Dynamo#34 to add unit and integration tests.

Declarations

Check these if you believe they are true

The codebase 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.
This PR modifies some build requirements and the readme is updated

…erator-tool

…nudsen/Dynamo into nmgt-nometadata

remove metadata references

add console colors for dictionary related errors update readme

mjkkirschner · 2021-08-20T15:41:56Z

src/DocumentationBrowserViewExtension/PackageDocumentationManager.cs

+
+            if (!string.IsNullOrEmpty(pathManager.HostApplicationDirectory))
+            {
+                var hostDir = new DirectoryInfo(Path.Combine(pathManager.HostApplicationDirectory, FALLBACK_DOC_DIRECTORY_NAME));


@sm6srw to answer your question about how useful this is - it actually uses the DynamoHostPath property as the root - which can be set using a startupconfiguration - for example, DynamoRevit sets this to the location of the DynamoRevit.dll entry point.

so I think this is pretty useful.

sm6srw

I have made a first pass but I need to come back and take another pass on all new files. Looks good. Just a few questions.

sm6srw · 2021-08-20T14:53:47Z

src/DocumentationBrowserViewExtension/PackageDocumentationManager.cs

        /// <param name="nodeNamespace">Namespace of the node to lookup documentation for</param>
        /// <returns></returns>
-        public string GetAnnotationDoc(string nodeNamespace)
+        public string GetAnnotationDoc(string nodeNamespace, string packageName)


Add packageName to param list?

yes @sm6srw, I had the same reaction initially, https://github.com/DynamoDS/Dynamo/pull/11725/files#r646888243 - but it's actually inside an extension, I think it's okay to break this with chance of risk and if possible it would be nice to decide and/or codify extensions being part of the API boundary or not but it will take a cross team effort / discussion I think.

in this case I'm okay with the change unless there are objections.

src/DynamoCLI/Program.cs

src/DynamoCore/Search/SearchElements/ZeroTouchSearchElement.cs

src/DynamoCore/Interfaces/LayoutSpecification.cs

src/DynamoCoreWpf/Interfaces/ILibraryViewCustomization.cs

src/DynamoCoreWpf/Properties/AssemblyInfo.cs

src/Tools/NodeDocumentationMarkdownGenerator/readme.md

move ilibraryviewcustomization as well to dynamocore

pinzart90 · 2021-08-23T13:30:43Z

so the what was the reason for the move from Wpf to Core ?

   <Compile Include="Interfaces\ILibraryViewCustomization.cs" />
    <Compile Include="Interfaces\LayoutSpecification.cs" />
    ```
    
 Was it because new projects that needed those interfaces had to depend on the dyn*wpf assembly ?
 And it was really no need to, since all the data in those 2 interfaces is pretty bare metal (no wpf) ?
 
 Also what is the main purpose of those interfaces ? TO provide a simple and uniform way for CustomViews to arrange their data  ?

pinzart90 · 2021-08-23T13:37:02Z

src/Tools/NodeDocumentationMarkdownGenerator/Commands/FromDirectoryCommand.cs

+
+        private static List<MdFileInfo> ScanFolderForAssemblies(string inputFolderPath, IEnumerable<string> filter, IEnumerable<string> referencePaths, SearchOption searchOption)
+        {
+            var allAssembliesFromInputFolder = Directory.GetFiles(inputFolderPath, "*.*", searchOption)


Not sure how many filles we expect to parse here.
EnumerateFiles might have some benefits...
The EnumerateFiles and GetFiles methods differ as follows: When you use EnumerateFiles, you can start enumerating the collection of names before the whole collection is returned; when you use GetFiles, you must wait for the whole array of names to be returned before you can access the array. Therefore, when you are working with many files and directories, EnumerateFiles can be more efficient.

I'd rather stick with the simplicity of GetFiles() - this is not going to affect users, and image compression time completely dwarfs the time it takes to read these files.

I'm thinking back to the last time I ran into trouble groking deferred execution and EnumerateFiles.

src/Tools/NodeDocumentationMarkdownGenerator/Commands/FromDirectoryCommand.cs

src/Tools/NodeDocumentationMarkdownGenerator/DynamoDictionaryEntry.cs

pinzart90 · 2021-08-23T14:19:31Z

src/Tools/NodeDocumentationMarkdownGenerator/NodeDocumentationMarkdownGenerator.csproj

+    <OutputType>Exe</OutputType>
+    <RootNamespace>NodeDocumentationMarkdownGenerator</RootNamespace>
+    <AssemblyName>NodeDocumentationMarkdownGenerator</AssemblyName>
+    <TargetFrameworkVersion>v4.8</TargetFrameworkVersion>


Looking at the PR and at the Readme (The NodeDocumentationGenerator is a CLI tool to generate node documentation markdown stubs.) it seems that there is no UI being done in this project.
What are the reasons for making this project use the full dotnet framework (instead of core or dotnet standard)

It's not clear to me that it's so simple to have a .net standard project depend on assemblies targeting .net framework (they are different targets after all) and this PR is big enough dissuade me from trying to also include multi-targeting our core dynamo assemblies for .net standard.

If I can simply change the target to .net standard and everything works I'm fine with that 😉 - otherwise I think it's probably best to include that improvement later.

from stack overflow

No, .NET Standard projects cannot reference framework projects. .NET Standard projects need to be usable across platforms, forcing a dependency on the .NET framework by referencing an assembly targeting it makes this impossible. Note that with some of the magic Microsoft is doing with .NET Standard 2.0 this is less true but the overall idea still stands.

this answer may be old, so I'll try it.

Yeah..dotnet standard probably cannot reference dotnet framework....
Unless we take on the gigantic task of braking apart the dependencies into standard vs non standard assemblies
Fine with this even if it stays as it is

pinzart90 · 2021-08-23T14:21:49Z

How are we going to test the new functionality ? Unit tests (with mocked input files....?) ?

mjkkirschner · 2021-08-23T14:56:03Z

so the what was the reason for the move from Wpf to Core ?

   <Compile Include="Interfaces\ILibraryViewCustomization.cs" />
    <Compile Include="Interfaces\LayoutSpecification.cs" />
    ```
    
 Was it because new projects that needed those interfaces had to depend on the dyn*wpf assembly ?
 And it was really no need to, since all the data in those 2 interfaces is pretty bare metal (no wpf) ?
 
 Also what is the main purpose of those interfaces ? TO provide a simple and uniform way for CustomViews to arrange their data  ?

I moved them because I wanted to

avoid referencing DynamoCoreWpf in this tool to optionally move it to mono,dotstandard etc.
The types incldue no references to WPF so I changed the namespace to reflect what they depend on, and the fact they are now in DynamoCore.dll

mjkkirschner · 2021-08-23T15:00:34Z

How are we going to test the new functionality ? Unit tests (with mocked input files....?) ?

I am working on a test PR here:
https://github.com/mjkkirschner/Dynamo/commits/nmgtunittestsmjk
based on Sylvester's work

pinzart90

LGTM

SHKnudsen added 30 commits April 14, 2021 16:22

initial commit

f9093e4

Use CLRDLLModule

354e682

Refactoring

3dd5ad4

add hosts flag and other additions to frompackage

05f5984

🧹🧹

d7fab57

Update Program.cs

f3c2bd0

clean up + handle overloads

bf971d0

🧹

e5ea6c2

Update AssemblyHandler.cs

70bf7c3

too many things 🤯

67353b4

Save images instead of embedding

999a0b5

comment updates

f8c13eb

add support for .ds files

7548c79

comments + move to tools folder

97d29e4

add reference to RestSharp - need to create PackageUploadRequestBody

7599dfb

Update FromPackageFolderCommand.cs

f1f5df5

Merge remote-tracking branch 'upstream/master' into Node-Markdown-gen…

a7b1542

…erator-tool

🧹

cc9705e

Remove logger

3cc3667

remove unused using

e372af6

Update FromDirectoryOptions.cs

62009d5

Update AssemblyHandler.cs

b3443ed

Change project type + add test project

f9a5627

remove old files

6772242

use flag to specify ReflectionContext

6328716

Merge remote-tracking branch 'upstream/master' into Node-Markdown-gen…

e2e3789

…erator-tool

few comment updates

9590d1a

Update DocumentationBrowserViewExtensionTests.cs

dbe0a36

ds file fixes

87fc191

fix add isReflection flag FFIMethodAtribute

2742147

SHKnudsen and others added 10 commits August 18, 2021 11:06

add dyf folder exists check

15cb6e6

Merge branch 'Node-Markdown-generator-tool' of https://github.com/SHK…

8a84827

…nudsen/Dynamo into nmgt-nometadata

resolve all reference paths

530b0fc

remove metadata references

comments

85a7d76

add image magick to license files

1faafb1

add readme

e5fd0da

add known issues section

6d9ae63

more readme tweaks

a0aa8cb

add verbose mode and some logging when scanning dlls

087bf81

add console colors for dictionary related errors update readme

add findings to known issues

e168952

mjkkirschner added the PTAL Please Take A Look 👀 label Aug 19, 2021

mjkkirschner requested review from aparajit-pratap, pinzart90 and sm6srw August 19, 2021 20:20

mjkkirschner commented Aug 20, 2021

View reviewed changes

sm6srw reviewed Aug 20, 2021

View reviewed changes

review comments

c228f59

move ilibraryviewcustomization as well to dynamocore

pinzart90 reviewed Aug 23, 2021

View reviewed changes

src/Tools/NodeDocumentationMarkdownGenerator/Commands/FromDirectoryCommand.cs Outdated Show resolved Hide resolved

pinzart90 reviewed Aug 23, 2021

View reviewed changes

src/Tools/NodeDocumentationMarkdownGenerator/DynamoDictionaryEntry.cs Show resolved Hide resolved

pinzart90 reviewed Aug 23, 2021

View reviewed changes

pinzart90 approved these changes Aug 23, 2021

View reviewed changes

review comments

943a9df

sm6srw approved these changes Aug 23, 2021

View reviewed changes

mjkkirschner merged commit e6b5ab3 into DynamoDS:master Aug 23, 2021

mjkkirschner deleted the nmgt-nometadata branch August 30, 2021 17:01

Conversation

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

Uh oh!

Purpose

Declarations

Uh oh!

mjkkirschner Aug 20, 2021 • 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!

sm6srw left a comment

Choose a reason for hiding this comment

Uh oh!

sm6srw Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Aug 20, 2021 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pinzart90 commented Aug 23, 2021

Uh oh!

pinzart90 Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pinzart90 Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Aug 23, 2021 • 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 23, 2021

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Aug 23, 2021

Choose a reason for hiding this comment

Uh oh!

pinzart90 Aug 23, 2021 • 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!

pinzart90 commented Aug 23, 2021

Uh oh!

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

Uh oh!

Uh oh!

mjkkirschner commented Aug 23, 2021

Uh oh!

pinzart90 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mjkkirschner commented Aug 19, 2021 •

edited

Loading

mjkkirschner Aug 20, 2021 •

edited

Loading

mjkkirschner Aug 20, 2021 •

edited

Loading

mjkkirschner Aug 23, 2021 •

edited

Loading

pinzart90 Aug 23, 2021 •

edited

Loading

mjkkirschner commented Aug 23, 2021 •

edited

Loading