Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Markdown output #169

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Markdown output #169

wants to merge 6 commits into from

Conversation

DrDub
Copy link

@DrDub DrDub commented Dec 12, 2022

This is PR allows PasDoc to generate Markdown output.

Markdown is supported natively by GitHub which makes reading the generated documentation visually pleasing, see the test cases in https://github.com/DrDub/pasdoc/tree/master/tests/testcases_output/markdown

Also, now that PasDoc supports Markdown in the comments, it can help transform complex comments into Markdown.

This PR has very small changes to PasDoc_GenHtml to re-use functionality.

It might be possible to further re-organize GenHtml to shrink GenMarkdown considerably but I don't feel comfortable doing that without extra consultation.

@michaliskambi
Copy link
Collaborator

This looks great! I will try to review it ASAP.

Initial comments:

  1. I see that you descend TMarkdownDocGenerator from TGenericHTMLDocGenerator.

    I see it resulted in a small modification to PasDoc_GenHtml.pas (to make some some stuff is protected instead of private) but in exchange it allowed a simpler code of TMarkdownDocGenerator than if you had to make it from generic TDocGenerator. It's a bit unexpected usage of TGenericHTMLDocGenerator but I fully agree with this approach.

    If you have ideas to rearrange TGenericHTMLDocGenerator more, to make it even better, I'd be happy to take them into account too :) Though the current state has a nice advantage that it's obvious that there's no regression in HTML generation.

  2. When generating anchors, remember to use Signature to generate different anchors for the overloaded methods. Recently our HTML generator had improvements related to it, and they should also work for Markdown.

    I wanted to check it, but I see https://github.com/DrDub/pasdoc/tree/master/tests/testcases_output/markdown doesn't contain subdirectory for ok_link_tag_overloads (but HTML subdirectory has it). Not sure why.

  3. Randomly looking at test output: https://github.com/DrDub/pasdoc/blob/master/tests/testcases_output/markdown/ok_auto_link/ok_auto_link.md has

    Self name: ok\_auto\_link
    

    That is, backslashes are visible. I'm guessing you add backslashes to the output to escape special underscore (_) meaning, but inside backticks the underscore is not special and backslashes are also not special -- so you should not add backslashes in this case.

Proper code review is coming! For now these are just quick comments :) Thank you!

@Fr0sT-Brutal
Copy link
Collaborator

Is it really necessary to inherit from HTML Gen? Probably it will be more logical to extract some common parts to a new base class?

@DrDub
Copy link
Author

DrDub commented Dec 25, 2022

Thanks for the warm comments. My answers below:

This looks great! I will try to review it ASAP.

Initial comments:

1. I see that you descend `TMarkdownDocGenerator` from `TGenericHTMLDocGenerator`.
   I see it resulted in a small modification to `PasDoc_GenHtml.pas` (to make some some stuff is `protected` instead of `private`) but in exchange it allowed a simpler code of `TMarkdownDocGenerator` than if you had to make it from generic `TDocGenerator`. It's a bit unexpected usage of `TGenericHTMLDocGenerator` but I fully agree with this approach.
   If you have ideas to rearrange `TGenericHTMLDocGenerator` more, to make it even better, I'd be happy to take them into account too :) Though the current state has a nice advantage that it's obvious that there's no regression in HTML generation.

Markdown format can take HTML. There are many things that PasDoc generates that cannot be done in vanilla markdown (tables with paragraphs inside, for instance or links in source code). If I don't derive the code from the HTML generator I'll need a separate instance to call in such situations.

2. When generating anchors, remember to use `Signature` to generate different anchors for the overloaded methods. Recently our HTML generator had improvements related to it, and they should also work for Markdown.
   I wanted to check it, but I see https://github.com/DrDub/pasdoc/tree/master/tests/testcases_output/markdown doesn't contain subdirectory for `ok_link_tag_overloads` (but HTML subdirectory has it). Not sure why.

Thanks for the heads up. I'll look for it and post questions about it, hopefully this week.

3. Randomly looking at test output: https://github.com/DrDub/pasdoc/blob/master/tests/testcases_output/markdown/ok_auto_link/ok_auto_link.md has
   ```
   Self name: ok\_auto\_link
   ```
   That is, backslashes are visible. I'm guessing you add backslashes to the output to escape special underscore (`_`) meaning, but inside backticks the underscore is _not_ special and backslashes are also not special -- so you should not add backslashes in this case.

This is a bug. I can go fix it and update this PR (unless you have already started the review, then I can include the bug fix with other changes).

@DrDub
Copy link
Author

DrDub commented Dec 25, 2022

Is it really necessary to inherit from HTML Gen? Probably it will be more logical to extract some common parts to a new base class?

I hear you. I also realize I did not answer @michaliskambi question:

If you have ideas to rearrange TGenericHTMLDocGenerator more, to make it even better, I'd be happy to take them into account too :) Though the current state has a nice advantage that it's obvious that there's no regression in HTML generation.

There are a few "heavy weight" methods in TGenericHTMLDocGenerator that contain multiple functions and procedures inside. The current code has tons of reduplication in the Markdown for a few small changes inside these large methods. It is a case where procedural Pascal doesn't play well with OOP.

Refactoring those methods to have their procedures outside them will allow for a quality base class. But the code will be less easy to read and bugs could be introduced.

@michaliskambi
Copy link
Collaborator

Sorry for falling silent around Christmas time, there was a lot of things to do (on computer and in the Real World) that kept me busy :)

I'd suggest:

  • @DrDub : Take care of the Signature and backslashes I mentioned above.
  • The inheritance from HTML generator is OK for me, at this point this is the easiest and maintainable route IMHO. Let's go with it -- for now, until we invent something better :)
  • I will try to finish the review later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants