Closed lovettchris closed 4 years ago
Thank you! I'll review ASAP.
Sorry for the delay on the review. This PR has many changes, and many of them are breaking changes. Existing clients are happy with the way the tool currently works. We use it as-is with GitHub Pages, which uses Jekyll, so these changes are not necessary for all Jekyll configurations. Some of the changes that need to be opt-in:
These are great contributions, but I can't justify breaking changes unless they are opt-in.
Thanks for your feedback. I will make the fixes you recommend. Some of them are already opt in. For example, see GetFileExtension in MarkdownGenerator, it only omits the file extension if you specify the new --front-matter
command line argument. Normally a github pages site that uses permalinks does not put file extensions in those permalinks. So if you specify front matter than you are building that type of github pages site. The reason being the .md is converted to .html in the final site, so the extension .md doesn't exist in that case.
I reversed --unbrowsable
to --skip-unbrowsable
so the default behavior remains unchanged and I added --namespace-pages
to make the generation of namespace pages optional.
I also added --skip-compiler-generated
to make this opt it although I'm not sure why you want to show all the C# generate get, set-, add* methods backing properties and events by default.
I'm not sure what you were referring to with this bullet, but the unit test passes now.
it only omits the file extension if you specify the new --front-matter command line argument
That certainly keeps it from being a breaking change, you're right. I'm not 100% convinced that an .md
extension user would never want front matter -- maybe they are just setting the layout -- but I can live with it.
I'm not sure why you want to show all the ... backing properties and events by default
Oof, you're right, I wasn't reading carefully enough; I was thinking "code generated". I never generate docs for private members so I've probably never seen them. Sorry, I agree, this doesn't even need to be an option. :blush:
suffixes on assembly/namespace/type file names
Sorry, I should have been clearer. I'm referring to adding Type
to the end of the file name for Markdown files describing types, e.g. MyClassType.md
instead of just MyClass.md
. Similarly with Assembly
and Namespace
. I don't want the default behavior to break links from the current implementation, which doesn't use the suffixes.
I need to document how to use the build script in CONTRIBUTING.md
. Running build test
(or ./build.sh test
) runs an additional test that confirms that the Markdown generator still works the same. (That's why the Appveyor PR build is failing.) When there are intentional changes, we run build generate-docs
to regenerate the docs; then build test
should succeed.
The stripping of periods (.Replace(".", "")
) also needs to be optional to preserve existing behavior.
Ok, so I did an experiment with Jekyll (which is what Github pages uses) and if I include the ".md" in the permalinks Jekyll generates the html pages with the .md file extension instead of .html which means the site doesn't work, I get the following. This is why I'm doing all this messing with file extensions. I can make this an option which I will call "--permalink pretty" to match up with the jekyll docs on the subject, and when this option is specified it triggers all the above behavior, and if not, then it leaves it as it was. But just note that this compatibility mode will not work in a github pages website.
So what tool are you using to generate your site at https://ejball.com/XmlDocMarkdown/ ?
I've fixed things so that "build test" passes without making any changes to your existing docs, except the new command line args. By the way I'm using https://jekyllrb.com/docs/github-pages/ to generate my site. Github uses Jekyll. See my example web site here. With github pages, I simply checkin the markdown and Github regenerates my site automatically (for free!). It's a great deal which is why I'm doing this work in the first place.
This PR provide support for providing "--frontmatter filename" which is a Jekyll front matter template for Github pages, like this:
It also modifies the file names produced so it is compatible with Jekyll. If you have class A with member M1 and M2 you used to get:
But if you want to turn these links into Jekyll "permalinks" it won't work because you can't have the top level ".../A" and another page with link ".../A/M1". This is a limitation of Jekyll I guess. So the solution is to append "Type" to the class documentation page so it doesn't conflict with the subdirectory name:
Similar problem happens with the Assembly level markdown, which needs to have the string "Assembly.md" appended so this file name doesn't conflict with a similarly named namespace subdirectory.
To make all this easier to manage I added a PageLocation to MarkdownContext, and a MakeRelative method that we can call, so we don't have to rememebr what the "relative" link is everywhere adding .. and ../.. and so on, it will compute the relative path correctly.
Lastly, one more limitation of Jekyll seems to be that the leaf file name cannot contain period, so something like "Foo.BarType" at the end of a permalink doesn't work for nested types, so the period are removed from the permalinks in this case, leaving FooBarType.
Here's a picture of how it can look inside a Github pages website: