Use a CLI tool for converting MD to HTML and for Sanitizing HTML as a way to not add any new 3rd party dependencies to Dynamo by sm6srw · Pull Request #11 · SHKnudsen/Dynamo

sm6srw · 2020-11-18T14:07:34Z

Purpose

This Pull Request addresses DYN-3243. Use a CLI tool for converting MD to HTML and for Sanitizing HTML as a way to not add any new 3rd party dependencies to Dynamo.

This Pull Request does

add a command line tool, Md2Html.exe, to Dynamo tools.
add a command line tool manager class Md2Html, to Dynamo utilities.
start using Md2Html for converting MD to Html and for Sanitizing Html in the document browser.
remove all 3rd party dependencies that was previously added to the document browser.

Notes:
The command line utility is still work in progress and will be finalized in a separate task.

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

Reviewers

@pinzart90 @mjkkirschner @aparajit-pratap

FYIs

@SHKnudsen

This reverts commit 99b42f2.

This reverts commit f655c39.

mjkkirschner · 2020-11-18T17:44:15Z

src/DocumentationBrowserViewExtension/DocumentationBrowserViewExtension.csproj

    <WarningLevel>4</WarningLevel>
  </PropertyGroup>
  <ItemGroup>
-    <Reference Include="AngleSharp, Version=0.14.0.0, Culture=neutral, PublicKeyToken=e83494dcdc6d31ea, processorArchitecture=MSIL">


sweet sweet assembly isolation. :)

mjkkirschner · 2020-11-18T17:48:02Z

src/DynamoUtilities/Md2Html.cs

+        /// <param name="writer"></param>
+        /// <param name="mdString"></param>
+        /// <param name="mdPath"></param>
+        public void ParseMd2Html(ref StringWriter writer, string mdString, string mdPath)


why use a ref in these parameters - you're not replacing the writer with another writer are you?

I am not. But you are right, this API can be simplified to just return a string.

mjkkirschner · 2020-11-18T17:48:46Z

src/DynamoUtilities/Md2Html.cs

+
+namespace Dynamo.Utilities
+{
+    public class Md2Html


do we expect others to use this utilty? If not maybe we make it internal?

Yes, internal makes more sense.

mjkkirschner · 2020-11-18T17:50:14Z

src/DynamoUtilities/Md2Html.cs

+        /// Destructor
+        /// Kill the CLI tool, if still running
+        /// </summary>
+        ~Md2Html()


finalizers.... hmmm
I think IDisposable has a better chance of being deterministic and controllable in testing scenarios -
just some thoughts:
https://www.viva64.com/en/b/0437/

mjkkirschner · 2020-11-18T17:50:33Z

src/DynamoUtilities/Md2Html.cs

+
+namespace Dynamo.Utilities
+{
+    public class Md2Html


curious what you think about making this IDisposable

I tried IDisposal and yes, much easier to control. I don't need the ProcessExit handler anymore. So I keep the IDisposal implementation for now.

mjkkirschner · 2020-11-18T17:51:30Z

src/Tools/Md2Html/MarkdownHandler.cs

+        /// referencing it with a relative path "./image.png", when we convert to html
+        /// we need the full path. This method finds relative image paths and converts them to absolute paths.
+        /// </summary>
+        private static void ConvertRelativeLocalImagePathsToAbsolute(string mdFilePath, MarkdownDocument document)


~~I feel we've written this method 100 times in the libraryUI at various times, let me see if I can find a version we can reuse/share.~~

no dice - nevermind.

mjkkirschner · 2020-11-18T17:52:26Z

src/Tools/Md2Html/Md2Html.csproj

+    <DebugSymbols>true</DebugSymbols>
+    <DebugType>full</DebugType>
+    <Optimize>false</Optimize>
+    <OutputPath>..\..\..\bin\AnyCPU\Debug\Md2Html\</OutputPath>


There is a props file that needs to be imported to set this automatically I think.

mjkkirschner · 2020-11-18T17:53:11Z

src/Tools/Md2Html/Program.cs

+{
+    static class Program
+    {
+        static void Main(string[] args)


have you considered using ndesk options in the repo to list CLI options / help docs

Nop, but I will take a look.

Added basic help

mjkkirschner · 2020-11-18T17:53:44Z

src/Tools/Md2Html/Properties/AssemblyInfo.cs

+// You can specify all the values or you can default the Build and Revision Numbers
+// by using the '*' as shown below:
+// [assembly: AssemblyVersion("1.0.*")]
+[assembly: AssemblyVersion("1.0.0.0")]


should we use the shared assembly generator to align the version number of this exe with the dynamo version number?

Yes, that make sense.

mjkkirschner · 2020-11-18T17:54:49Z

src/Dynamo.All.sln

 EndProject
 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "NodeAutoCompleteViewExtension", "NodeAutoCompleteViewExtension\NodeAutoCompleteViewExtension.csproj", "{51511AFD-F326-4995-8E27-5D711419EF6F}"
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Md2Html", "Tools\MD2HTML\Md2Html.csproj", "{0893F745-CB1A-427A-8E87-CA927273039A}"


if this tool is cross platform does it make sense to add it to the mono.sln as well?

I will give it a try and see if it works in mono.

Mono do compile so keep for now. I will address your other cross platform comments next.

mjkkirschner · 2020-11-18T21:10:50Z

src/Tools/Md2Html/MarkdownHandler.cs

+
+            foreach (var image in imageLinks)
+            {
+                if (!image.Url.StartsWith("./"))


can we changes this to Uri.IsWellFormedUriString(url, UriKind.Relative) - otherwise this is windows only.

mjkkirschner · 2020-11-18T21:11:50Z

src/Tools/Md2Html/MarkdownHandler.cs

+                if (!image.Url.StartsWith("./"))
+                    continue;
+
+                var imageName = image.Url.Split(new string[] { "./" }, StringSplitOptions.None);


can we use the system.io.path options to get the filename instead - that will be crossplatform.

…nudsen/Dynamo into DYN-3243

sm6srw · 2020-11-20T14:11:40Z

@pinzart90 @mjkkirschner PTAL!

mjkkirschner · 2020-11-20T18:02:27Z

src/DynamoUtilities/Md2Html.cs

+        /// Read data from CLI tool
+        /// <param name="writer"></param>
+        /// </summary>
+        private void GetData(ref StringWriter writer)


still using ref? Intentional?

Old habit. I will get rid of it.

mjkkirschner · 2020-11-20T18:04:25Z

src/DynamoUtilities/Md2Html.cs

+        /// <returns>Returns full path to the CLI tool</returns>
+        private static string GetToolPath ()
+        {
+            var rootPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) ?? throw new ArgumentNullException("Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)");


you can probably use nameof here?

…nudsen/Dynamo into DYN-3243

mjkkirschner

it looks good - my biggest concern still is the use of ref - it seems unnecessary / confusing.

mjkkirschner · 2020-11-20T18:06:10Z

src/DynamoUtilities/Md2Html.cs

+        private static string GetToolPath ()
+        {
+            var rootPath = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location) ?? throw new ArgumentNullException("Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)");
+            var toolPath = Path.Combine(rootPath, @"Md2Html\Md2Html.exe");


I don't know the answer to this - does Path.Combine generate a valid path cross platform even if you use hardcoded directory separator characters?

Good point. Let me fix that.

pinzart90 · 2020-11-20T18:10:50Z

src/Tools/Md2Html/Program.cs

+
+            while (true)
+            {
+                var line = Console.ReadLine();


should we handle exceptions inside the while(true) ? if exception is thrown...it will kill the process right ?

Yes good point. We should exit the process if anything unexpected happens with the communication.

sm6srw · 2020-11-20T20:00:47Z

@mjkkirschner @pinzart90 PTAL!

mjkkirschner · 2020-11-20T20:23:06Z

src/DynamoUtilities/Md2Html.cs

+        /// <returns>Returns error message</returns>
+        private string GetCantCommunicateErrorMessage()
+        {
+            return @"<p>Can't communicate with '" + GetToolPath() + @"'</p>";


last thing - is this intended for us or for users? If it's for users then we should put it in a resx file which the localization team can localize.

They will show up in the document browser window.

I will work adding resources for these strings.

pinzart90

LGTM

sm6srw · 2020-11-23T15:47:26Z

@mjkkirschner Move error messages to resources. PTAL!

pinzart90

LGTM

mjkkirschner · 2020-11-23T16:00:54Z

LGTM

* add packageDocManager to handle package documentation links * new viewModelEventArgs to send node annotation data to doc browser * Handle node annotation in doc browser * support custom NodeModels + fix watcher event bug * Update NodeAnnotationNotFound.md * Update NodeAnnotationNotFound.md * Comment updates * Update AssemblySharedInfo.cs * Update src/DynamoCoreWpf/ViewModels/Core/DynamoViewModelEventArgs.cs * addtional comment updates + clean up * clean up packageDocManager * Update DocumentationBrowserViewModel.cs * handle overloads + fix hot reload bug * use HtmlSanitizer to clean potential dangerous content from markdown files * move PackageDocumentationManager to DocumentationBrowser * update syntax highlighting + markdownStyling updates * comment updates * add Markdig to License files * Change GetPackageName to GetMainCategory * update markdig + htmlsanitizer version * fix failing tests + add new node documentaion tests * fix broken test * update link to wiki * Update DocumentationBrowserViewExtension.cs * Add MD2HTML Tool * First pass - singleton version * Revert "Add MD2HTML Tool" This reverts commit 71509da. * Revert "First pass - singleton version" This reverts commit bbefc36. * fix test * add doc folder to PackageDirectoryBuilder (#10) * add doc folder to PackageDirectoryBuilder * update tests * comment update * Use a CLI tool for converting MD to HTML and for Sanitizing HTML as a way to not add any new 3rd party dependencies to Dynamo (#11) * Revert "Revert "Add MD2HTML Tool"" This reverts commit 99b42f2. * Revert "Revert "First pass - singleton version"" This reverts commit f655c39. * Implement HTML Sanitizer * Remove unneeded dependecies from the doc extension * Cleanup naming * More naming cleanups * Rename files * Rename folder * Add comments * Use literal strings * Hardening wrapper class * Simplify API * Make wrapper API internal * Add Tools project to Mono solution * Fix directory name (case) * Add missing internal * Use IDispose pattern * Fix literal string * Fix cross platform issues * Use CS.props file * Undo change to AssemblySharedInfo.cs * Use shared assembly info * Add more docs * Add ndesk support and implement help option * Rename class * Add missing change * Remove the use of ref * Fix typo * Fix toolpath and use nameof() * Improve error handling * Move strings to resources * Enhance error messages * Merge master (#12) * Fix sorting for number keys in List.SortByKey (DynamoDS#11241) * fix sorting for number keys in List.SortByKey * add unit test * Crash when accessing static hidden methods from zero touch nodes (DynamoDS#11238) * handle hidden static methods from zero touch nodes * add images (DynamoDS#11243) * Revert "Adding copy of missing file from LibG nuget package (DynamoDS#11218)" (DynamoDS#11254) This reverts commit a71b5c8. * Revert "Revert "Adding copy of missing file from LibG nuget package (DynamoDS#11218)" (DynamoDS#11254)" (DynamoDS#11255) This reverts commit 257ac14. * Undock/redock extension tabs to windows (DynamoDS#11246) * Somewhat working version * Pretty much working version * Finishing touches and test * Cleanup * Prevent both window and tab * Improve method name and comment * Forgot to include this in last commit * add analytics to extensions load event (DynamoDS#11258) * Revert Project Reference Changes (DynamoDS#11256) * Initial Commit * fix mono build * Update to latest libG nuget version * Cleanup entries VS Studio failed to update * Fix Mono build * Add the private flag back (modified by VS Studio) * Regressions * Add back missing resource * Change DefaultValue setter to public (DynamoDS#11251) * update load asm binaries from registry logic (DynamoDS#11261) * update load asm binaries from registry * Add coverage for docking (DynamoDS#11262) * Adding a public API on the View extension that gets triggered when the view extension is closed. (DynamoDS#11260) * Adding a public API on the View extension that gets triggered when the view extension is closed. * Update variable name * changing the name to ViewExtensionBaseClass * Addressing some comments. * Adding unit test. * Changing the name to Closed() * Some more comments. * Adding an additional test * Adding checks for the Menu items before setting its value. * Update Style (DynamoDS#11263) * CustomNodes that replicate do not access trace correctly. (DynamoDS#11244) * add failing test * this works, but seems like cheating. more tests * new tests, one failing, needs discussion * fix test - still fails but as expected * tests all pass with this change - but looking for better alternatives * add test use new field - cant find a better way currently add comments * revert internal update test * reset guid map before each test * failing test * reset * whitespace * Localize buttons (DynamoDS#11265) * update version num and tt file (DynamoDS#11269) * Fix merge error * Fix merge error * Revert "Fix merge error" This reverts commit 0a21ca4. Co-authored-by: aparajit-pratap <[email protected]> Co-authored-by: pinzart90 <[email protected]> Co-authored-by: Sylvester Knudsen <[email protected]> Co-authored-by: Aaron (Qilong) <[email protected]> Co-authored-by: Michael Kirschner <[email protected]> Co-authored-by: Martin Misol Monzo <[email protected]> Co-authored-by: Ashish Aggarwal <[email protected]> Co-authored-by: reddyashish <[email protected]> * Extract more strings (#13) * Fix failing tests (#14) * Fix failing tests * Revert "Fix failing tests" This reverts commit 824f785. * Fix failing tests * Properly dispose all string writers * More failing test fixes (#15) * Fix for regression in multi-output node preview (DynamoDS#11266) * fix for multi-output node preview regression * update tests * add test * revert unwanted changes * update test * Life cycle refactoring: Get rid of singleton. Let the model create and own the MarkdownHandler on first use. * Fix crash in some document browser tests * Properly dispose all DocumentBrowserViewExtension objects * Revert "Fix crash in some document browser tests" This reverts commit ab146f9. * Display input port type specific default suggestions only (DynamoDS#11268) * input port type specific default suggestions * test fix * Test fixes * Use InTestMode flag * Delete empty lines * turn off hit testing visibility for DM related render packages (DynamoDS#11282) * Apply syntax highlighting to multi-line strings (DynamoDS#11289) * Also sanitize node documentation html * Save/Load view extension size and location (DynamoDS#11278) Information of where an extension control was last shown is saved and retrieved as preference settings. These are actually not directly visible for the user and are updating automatically, just like the main Dynamo window size. These settings are used when showing the extension control, to restore it to how it used to be before closing it. The available information for each view extension is: 1. How it is shown. For now this can be either as a floating window or docked to the right-side panel. 2. In case it is shown as a floating window, the following information about the window is saved: - Location by Top/Left coordinates - Size by Width/Height - Whether the window is maximized * add revit dlls to assembly conflict checker ignore list (DynamoDS#11285) * Update StartupUtils.cs Co-authored-by: pinzart <[email protected]> * Fix sanitizing log warnings * Node AutoComplete suggestions sorted alphabetically within the same group (DynamoDS#11280) * add/update tests * Address review comments * Add ILMerge * Fix typo * Fix mono build * Revert "Fix mono build" This reverts commit 9904525. * Refine mono solution * Remove task condition * Mono build fix * Add Readme Co-authored-by: aparajit-pratap <[email protected]> Co-authored-by: Ashish Aggarwal <[email protected]> Co-authored-by: Laurence Elsdon <[email protected]> Co-authored-by: Martin Misol Monzo <[email protected]> Co-authored-by: pinzart90 <[email protected]> Co-authored-by: pinzart <[email protected]> Co-authored-by: Radu Gidei <[email protected]> Co-authored-by: Jorgen Dahl <[email protected]> Co-authored-by: Jorgen Dahl <[email protected]> Co-authored-by: aparajit-pratap <[email protected]> Co-authored-by: pinzart90 <[email protected]> Co-authored-by: Aaron (Qilong) <[email protected]> Co-authored-by: Michael Kirschner <[email protected]> Co-authored-by: Martin Misol Monzo <[email protected]> Co-authored-by: Ashish Aggarwal <[email protected]> Co-authored-by: reddyashish <[email protected]> Co-authored-by: Laurence Elsdon <[email protected]> Co-authored-by: pinzart <[email protected]>

sm6srw added 11 commits November 17, 2020 11:39

Revert "Revert "Add MD2HTML Tool""

3bd6e06

This reverts commit 99b42f2.

Revert "Revert "First pass - singleton version""

28588b7

This reverts commit f655c39.

Implement HTML Sanitizer

fbef96f

Remove unneeded dependecies from the doc extension

7ace826

Cleanup naming

0c7a5f2

More naming cleanups

f53ffce

Rename files

41c9d25

Rename folder

770fa7e

Add comments

2b76b86

Use literal strings

c8101c3

Hardening wrapper class

cb20ea3

mjkkirschner reviewed Nov 18, 2020

View reviewed changes

sm6srw added 5 commits November 18, 2020 14:54

Simplify API

b2a2a1e

Make wrapper API internal

c422314

Add Tools project to Mono solution

0851717

Fix directory name (case)

8303247

Add missing internal

9927831

mjkkirschner reviewed Nov 18, 2020

View reviewed changes

sm6srw added 2 commits November 18, 2020 16:12

Use IDispose pattern

e1e81ce

Fix literal string

20ce8d3

sm6srw added 9 commits November 19, 2020 09:46

Fix cross platform issues

dba5033

Use CS.props file

67a0f14

Undo change to AssemblySharedInfo.cs

c9f2231

Use shared assembly info

f87a120

Add more docs

abe1755

Add ndesk support and implement help option

bf4a74b

Rename class

96b5fc0

Add missing change

ed2e5bf

Merge branch 'Node-annotations-doc-browser' of https://github.com/SHK…

aee38b4

…nudsen/Dynamo into DYN-3243

mjkkirschner reviewed Nov 20, 2020

View reviewed changes

Merge branch 'Node-annotations-doc-browser' of https://github.com/SHK…

f09e522

…nudsen/Dynamo into DYN-3243

mjkkirschner approved these changes Nov 20, 2020

View reviewed changes

pinzart90 reviewed Nov 20, 2020

View reviewed changes

sm6srw added 4 commits November 20, 2020 14:00

Remove the use of ref

db77dc2

Fix typo

fdbbdef

Fix toolpath and use nameof()

51ee2d9

Improve error handling

b97a89b

mjkkirschner reviewed Nov 20, 2020

View reviewed changes

pinzart90 approved these changes Nov 20, 2020

View reviewed changes

sm6srw added 2 commits November 23, 2020 10:26

Move strings to resources

261bc71

Enhance error messages

c0818d4

pinzart90 approved these changes Nov 23, 2020

View reviewed changes

mjkkirschner merged commit c8cf9f3 into SHKnudsen:Node-annotations-doc-browser Nov 23, 2020

sm6srw deleted the DYN-3243 branch December 3, 2020 23:03

Conversation

sm6srw commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

This Pull Request does

Declarations

Reviewers

FYIs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Nov 18, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm6srw Nov 18, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm6srw commented Nov 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sm6srw commented Nov 18, 2020 •

edited

Loading

mjkkirschner Nov 18, 2020 •

edited

Loading

sm6srw Nov 18, 2020 •

edited

Loading