Added MSBuild Task to generate and merge markdown #17
Conversation
Added Task. Included file merge also.
added mock build engine and real TaskItem instances to a test on the new task.
Test Passes.
Added custom task to .targets file. Removed redundant elements of target.
updated nuspec. Should point files back to tools directory though.
slight changes to paths.
This file is generated during build. Not needed.
Generated during build. Not needed.
mentions build task.
updated nuspec and updated assembly info on both msbuild task and executable.
added flag for msbuild task. warn rather than error when set to true and unexpected tag is encountered.
|
Resolves #12. Manually tested and in unit tests. |
|
Accidentally closed PR. |
|
Sorry, I've been off GH for a bit. This is really cool and is a lot to take in... if I'm reading this right, though, won't it break for existing users? If they don't have a readme.md in their project today it won't find it and it will error out. |
|
Yeah--it probably needs the default functionality to handle all existing use cases without failure and without reconfiguration. I'd have to look at it for a second to see whether that's what would happen. |
|
It looks like the only potential issue would be updating the NuGet package in projects where it is already installed. |
|
I would be very tempted to make the "merge" functionality politely ignore the absense of the readme file. You'd still have the small breakage of existing users that their XmlDoc would now contain their Readme files, but you wouldn't have existing users seeing the project break for them. I've been looking into how to make this configurable - there's not much documentation on Nuget build/.targets and build/.props files, but it looks like .props aren't user friendly and are a bit coarse. I wouldn't want to include breaking functionality without an ability to turn it off. |
|
Suppressing some of the build tasks might work. Turn that on and you get whatever functionality you want rather than the default. Heck, there could be 3 or 4 alternative project files--supposing there are actually that many viable choices. |
|
And by a small set of notes I mean something like:
|
|
I'm not sure I'd want users to be forced to edit the .csproj file since it's such an opaque mess, but I know that's the way this is supposed to be done and the future of this stuff. It's too bad there's no way to apply XML comments at the Assembly level (only to classes/props/etc.) or I'd say put the config there - an assembly-level Readme.md would've been good. It's tempting just to create a XmlCommentMarkdownGenerator.Config.xml (or json, as you like it) at the root of the target project and just keep settings there... but that seems less idiomatic than just editing the .csproj. Or we could put YAML front-matter in the project's readme.md for parameters, Jekyll-style. It's pretty standard approach, but it would mean a bunch of YAML crap in the HTML view if you opened the readme.md Like start the readme.md with And assume the user wants to not merge the readme if that block is missing. |
|
Okay. Using front-matter makes sense and it's a frequently used pattern. I'll add a commit or two to handle it. |
A couple items of note: - Code52/pretzel is a partial .NET implementation of Jekyll. - Process() apparently is always called. - CanProcess() is only called as needed based on the file timestamps. - After the custom tag check, the MD file merging should occur. - `pretzel taste` may need some try/catch or other handling. Below is the output when run against my test content: ``` >pretzel taste -t=customxmlmd starting pretzel... taste - testing a site locally About to check CanProcess(context) again about to check 'mergexmlcomments' a bool as boolean 'mergexmlcomments' is true Custom Tag to allow: all Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object. at Pretzel.Logic.Minification.LessTransform.<Transform>b__5_0(Page p) at System.Linq.Enumerable.WhereListIterator`1.MoveNext() at Pretzel.Logic.Minification.LessTransform.Transform(SiteContext siteContext) at Pretzel.Commands.TasteCommand.Execute(IEnumerable`1 arguments) at Pretzel.Program.Run(BaseParameters baseParameters) at Pretzel.Program.Main(String[] args) ```
These values are read by the custom plugin.
Bad News/Good NewsCurrently
|
Using the Pretzel Adapter
|
|
Ooooof, can Pretzel is far too big of a dependency. I hate to keep poo-pooing this great work you're doing, but that's overkill. Detect the opening "---", then find the next line that starts with "---", and apply a YAML parser to everything in between to pull out the config fields. |
|
Alright. I’ll go with pull the front matter directly. |
|
Tested. |
Pxtl
left a comment
Pxtl
left a comment
There was a problem hiding this comment.
Looks good - there's a bit of naming I don't like, and some Pretzel stuff is still there.
I'm mostly concerned about getting the public file config API right so that users don't have to deal with that churning if we have to refactor later.
Thanks for doing so much on this, it looks great.
| @@ -0,0 +1,60 @@ | |||
| using Pretzel.Logic.Templating; | |||
There was a problem hiding this comment.
Is this file still being used? I thought the new workflow got rid of this.
There was a problem hiding this comment.
- No, not used.
- Yes, it has been gotten rid of.
- The delete didn't get committed apparently.
| @@ -0,0 +1,20 @@ | |||
| --- | |||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
| //using System.Threading.Tasks; |
| /// Defaults to false. When true unexpected tags in the documentation | ||
| /// will generate warnings rather than errors. | ||
| /// </summary> | ||
| public bool WarnOnUnexpectedTag { get; set; } = false; |
There was a problem hiding this comment.
The inconsistent naming between the Enum, the Bool, and the Enum type itself is problematic but I'd rather just put this in and refactor that later.
| /// <summary> | ||
| /// The file to be created by the merge. Unused if MergeFiles evaluates to false. | ||
| /// </summary> | ||
| public ITaskItem OutputFile { get; set; } |
There was a problem hiding this comment.
Because it's so tightly tied to MergeFiles behavior, rename to MergedOutputFile. Doubly confusing because you also have an OutputPath property below that has a completely unrelated behavior.
| @@ -0,0 +1,273 @@ | |||
| using System; | |||
There was a problem hiding this comment.
General problem in this file: You have 3 config paths. InputXml (an array of paths to XML files), DocumentationPath (a path to a markdown file or folder) and OutputFile (a path to the output file to be created). This naming is very inconsistent - they're all paths (although the Input and Output ones are to a file while DocumentationPath is permitted to be a folder). Also, DocumentationPath wears two hats - it's both the folder the pre-merged documentation gets written to and the folder where the MD files get output to before they merged into the single giant MD document. Many projects have their readmes stored outside of the ./Docs folder, and so it would make sense to have those feed into the ./Docs folder output.
Imho, split the documentation path into two folders.
Clearer naming would also definitely help. My recommendations:
InputMdDocumentationPath,
OutputMdDocumentationPath,
MergedOutputFilePath,
InputXmlFilePaths.
I prefer clear verbosity to terse ambiguity.
Also, I'm looking at the behavior here: it looks like if the DocumentationPath is a file and merge is off, it will read from that file to get parameters, and then replace the body of that file with the generated documentation. That seems bad - really highlights the need to separate the input DocumentationPath and the output one.
|
|
||
| /// <summary> | ||
| /// DocumentationPath is the top level directory in which to search for files. | ||
| /// It is also the path where generated markdown files are created. |
There was a problem hiding this comment.
This comment highlights the problem of how DocumentationPath wears two hats - it does not make sense that the header docs must be present in the same ./Docs folder they're going to get merged into.
| GenerateFiles(); | ||
| if (MergeFiles) | ||
| { | ||
| Merge(); |
There was a problem hiding this comment.
Shouldn't there be a separate CreateDirectoryIfNeeded for outputfile because it's possible that the merged outputfile is outside of documentation path?
| var mdFiles = Directory.EnumerateFiles(DocumentationPath.ItemSpec, "*.md", SearchOption.AllDirectories).ToList(); | ||
| foreach (var mdFile in mdFiles) | ||
| { | ||
| if (TryGetFrontMatter(mdFile, out string frontMatter, out bool isEmpty) && |
There was a problem hiding this comment.
I'm not fond of the fact that it does this in apparently undefined order isntead of looking for an .md file that matches the XML file by name first, or look fora file named readme.md first... but either way, it's good for now. This looks like an opportunity for future enhancement.
2 participants
This change allows multiple input XML files and also merging of existing (manually maintained) markdown files. Added a test project for the MSBuild task. The AfterBuild target in .targets file is reduced to two steps:
It also resolves #16.
Tested with RhinoMocks and MSTest. Also tested the .nupkg file in VS.