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

extensions: overhaul javadoc/doxygen comment parsing #216

Merged
merged 4 commits into from
Nov 17, 2023
Merged

Conversation

jnikula
Copy link
Owner

@jnikula jnikula commented Nov 15, 2023

Okay, I never thought I'd put much effort into this, but the current javadoc/doxygen one is just embarrasing. Some might argue not implementing a parser using a parser generator is also embarrassing, but I'm really not aiming that high. A few hundred lines of bespoke parser to bridge the gap should be good enough.


Full javadoc/doxygen compatibility has never been a goal for hawkmoth. We've always promoted using pure reStructuredText and Sphinx, because it avoids any problematic conversions. If you want all the bells and whistles of doxygen, you should use doxygen.

However, we have to acknowledge there are a lot of codebases full of javadoc/doxygen style documentation comments, and a lot of people who are familiar with that style of code documentation. Requiring the use of reStructuredText to even try hawkmoth can be quite a hurdle.

To that end, we've always had a rudimentary regex based conversion available. But let's face the fact, is too simple, and too difficult to maintain or extend as it is.

Try to find a middle ground with an improved parser that understands paragraphs, inline markup, code blocks, and the like. Make it easier to extend. Recognize all doxygen commands (more than 180 of them!), even though we only implement a handful, making it possible to warn about the unimplemented ones (this is for future improvement, not done yet).

Copy link
Collaborator

@BrunoMSantos BrunoMSantos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow... You know, this may well be the most impactful commit yet, which is kind of ironic because it's an extension neither of us personally care about that much 😅

Didn't go about checking this against Doxygen documentation or anything or even testing things beyond the tests we have so far, but looks alright. No notes.

Not sure how confident you are on merging this right ahead... Are there any known users? Can we get them to try it out?

@jnikula
Copy link
Owner Author

jnikula commented Nov 16, 2023

Wow... You know, this may well be the most impactful commit yet, which is kind of ironic because it's an extension neither of us personally care about that much 😅

Indeed!

Didn't go about checking this against Doxygen documentation or anything or even testing things beyond the tests we have so far, but looks alright. No notes.

I really didn't expect that either. The number of doxygen commands blew my mind. And kind of reaffirmed my belief that if you want to incorporate API documentation to Sphinx, Hawkmoth is the way to go!

Not sure how confident you are on merging this right ahead... Are there any known users? Can we get them to try it out?

I do want to add more tests before merging. Probably not for everything, because that's just too overwhelming, but maybe I'll sample the source for some of the highlighted projects at https://doxygen.nl/results.html and make sure the popular things work. I already looked at dbus, and I was actually surprised how basic the usage was. Another approach is to look at coverage and add targeted tests to increase coverage; there's quite a bit of reuse for the "handlers".

On the other hand, the old one was so rudimentary that this can't possibly be worse! 😆

Anyway, I did ask for feedback at #210 (comment)

@BrunoMSantos
Copy link
Collaborator

Not sure how confident you are on merging this right ahead... Are there any known users? Can we get them to try it out?

I do want to add more tests before merging. Probably not for everything, because that's just too overwhelming, but maybe I'll sample the source for some of the highlighted projects at https://doxygen.nl/results.html and make sure the popular things work.

👍

I already looked at dbus, and I was actually surprised how basic the usage was.

That actually doesn't surprise me. I expect most projects are very frugal with what they use out of discipline / consistency but also probably due to no one being able to remember and use all those things effectively.

Maybe you can look at doxygen itself? That might be the most comprehensive example. In fact they may have unit tests of their own we can poach, license allowing.

Another approach is to look at coverage and add targeted tests to increase coverage; there's quite a bit of reuse for the "handlers".

Yeah, though it does beg the age-old question as to what coverage tells us apart from identifying dead code <.<

On the other hand, the old one was so rudimentary that this can't possibly be worse! 😆

Bold last words, but yeah :P

@jnikula
Copy link
Owner Author

jnikula commented Nov 16, 2023

Maybe you can look at doxygen itself? That might be the most comprehensive example. In fact they may have unit tests of their own we can poach, license allowing.

Yeah, there's a license conflict. Can't use GPL2 code in a BSD licensed project.

Full javadoc/doxygen compatibility has never been a goal for
hawkmoth. We've always promoted using pure reStructuredText and Sphinx,
because it avoids any problematic conversions. If you want all the bells
and whistles of doxygen, you should use doxygen.

However, we have to acknowledge there are a lot of codebases full of
javadoc/doxygen style documentation comments, and a lot of people who
are familiar with that style of code documentation. Requiring the use of
reStructuredText to even try hawkmoth can be quite a hurdle.

To that end, we've always had a rudimentary regex based conversion
available. But let's face the fact, is too simple, and too difficult to
maintain or extend as it is.

Try to find a middle ground with an improved parser that understands
paragraphs, inline markup, code blocks, and the like. Make it easier to
extend. Recognize all doxygen commands (more than 180 of them!), even
though we only implement a handful, making it possible to warn about the
unimplemented ones (this is for future improvement, not done yet).
Improve the description of the javadoc builtin extension. This also
expands the tests slightly.
It's a little better than "rudimentary at best", but still keep the
expectations at bay.
@jnikula
Copy link
Owner Author

jnikula commented Nov 17, 2023

Slightly tweaked the inline markup regexps, dropped the "split field list" thing after experimenting with what doxygen actually does, and added some documentation commits on top. I expanded the example, which also means it slightly expands the test coverage. I could add a lot more tests, but not sure I want to put a huge amount of effort into it yet.

@BrunoMSantos
Copy link
Collaborator

Looks good to me.

@jnikula
Copy link
Owner Author

jnikula commented Nov 17, 2023

Looks good to me.

Thanks! Let's merge, and iterate from there as needed!

@jnikula jnikula merged commit f2a249e into master Nov 17, 2023
5 checks passed
@jnikula jnikula deleted the javadoc-rfc branch November 18, 2023 18:20
@stephanlachnit
Copy link
Contributor

Tested it on my current project, and it works perfectly 🎉

@jnikula
Copy link
Owner Author

jnikula commented Nov 19, 2023

Tested it on my current project, and it works perfectly 🎉

That's awesome! Thanks for testing.

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.

3 participants