feat: Javadoc parser submodule

[I-Al-Istannen]

Grube!

This PR aims to close #4717 by providing a more fully-fledged javadoc parser. Currently the javadoc model is read-only but a small JavadocVisitor that writes out a new model to a string is absolutely possible and I created a not-quite-compliant one for testing:

Current structure

(Green is interface realization, blue is inheritance, white is aggregation/association. Pink is an Enum, dark blue is a class, light blue is an interface)

The main model consists of a JavadocReference (which represents references in @link and @see tags), a JavadocText for plain old (html or not) text, a JavadocInlineTag for inline tags and a JavadocBlockTag for block tags. The inline tag was specialized in the JavadocSnippetTag, as that provides additional attributes.

The main entrypoint is the JavadocParser, which takes a String and a CtElement and spits out a list of elements describing the javadoc, utilizing the InlineTagParser and BlockTagParser in the process. The JavadocSnippetBody has a factory method for parsing and creating it from a String (e.g. obtained from a snippet tag or a regular java file).

The link resolving is really lenient and mostly ignores the parameters if they don't seem to fit. While this doesn't really make sense, Javadoc seems to do the same and existing docs rely on the doclet fixing them.

Review

This seems to work just fine for my Javadoc API: I am able to do what I need and convert it to my own datamodel. Having the references resolved is quite nice and seems to work okay.

I would like some feedback on the concept and design (less so a detailed code review at this stage). Maybe you see some issue or have your own use cases we could vet the model against. It isn't surprising that it handles my usecase, as I wrote both of them :P

I think I might want to add a Function<CtType<?>, Collection<CtExecutableReference<?>>> which executes CtType#getAllExecutables() so clients can cache it. This speeds things up considerably, but it also makes the API a bit weirder. Not sure if exposing that is a good idea.

[slarse]

I've wandered in here around once a week and each time I see the 2k added lines I silently back out of the room :)

I think we should absolutely make an effort to integrate something like this, it's a very nice feature to have, but we're going to have to do a piecewise integration.

Alternatively, perhaps this is something we could offer outside of "core Spoon", as some form of plugin or the like. Based on absolutely no empirical data, I'm thinking that most consumers of Spoon don't parse Javadoc. Not that we have the capacity for plugins at the moment, but perhaps we should.

[I-Al-Istannen]

I've wandered in here around once a week and each time I see the 2k added lines I silently back out of the room :)

Yea, I am aware of that and it is why the title mentions "design". A codereview isn't the most critical, I was mainly looking for feedback on whether the API looks obviously wrong or misleading.

Alternatively, perhaps this is something we could offer outside of "core Spoon", as some form of plugin or the like. Based on absolutely no empirical data, I'm thinking that most consumers of Spoon don't parse Javadoc. Not that we have the capacity for plugins at the moment, but perhaps we should.

That sounds like a good idea IMHO. It intentionally has no tight integration with spoon (as evidenced by the -0 up there), as long as you throw it the raw Javadoc content and the CtElement it will happily do its job. Coincidentally, this is currently broken if you pass in a ZIP to the Launcher: The handling of zip files is broken at the moment and swallows raw javadoc contents.

I would be fine with properly extracting this in a different module and adding a runtime dependency on spoon in there. Then we could have a small pointer somewhere (maybe in the get javadoc method?) that a more fully-fledged javadoc api is available outside of the core package? Not sure how high the demand is, yea :)

[MartinWitt]

Alternatively, perhaps this is something we could offer outside of "core Spoon", as some form of plugin or the like.

We could place this plugin inside the spoon-labs organization and handle the same as the spoon-maven-plugin? Issues here, PRs inside the repo. Or @I-Al-Istannen has this code as a self owned repository.

Not that we have the capacity for plugins at the moment, but perhaps we should.

Do we really want to support a real plugin system? This seems like a lot of effort, for exactly one plugin. We could simply add a link in the documentation to the repo with full fledge Javadoc parser.

Not sure how high the demand is, yea :)

New features bring new ideas. Perhaps someone will notice the new capability and create new and exciting things with it. Could you create a markdown file or something for the /doc folder which showcases how to use the parser?

[I-Al-Istannen]

I have changed this to be a proper child of spoon-pom and also made spoon-pom aware of spoon-core. The version number is now maintained as a property in spoon-pom to not duplicate it across multiple files.

To properly build everything, mvn should be run from inside the spoon-pom folder and the reactor build will take over and build spoon-core and spoon-javadoc.

There is currently no protocol to have submodules (or build or publish them), so I am not sure what we want to do here. The whole sub module should probably be either tagged experimental or have its version set to something similar.

[MartinWitt]

Sorry if this was dormant for a bit. Large PRs always scare me. As this is a massive feature, I won’t merge this without the other integrators also giving green light. Let's first discuss the remaining organizational stuff and review the code later?

The whole sub module should probably be either tagged experimental or have its version set to something similar.

Yes, mark it as experimental and indicate it with a version number below 1.0 this. This gives us(you) room to change things in the future.

There is currently no protocol to have submodules (or build or publish them), so I am not sure what we want to do here.

Releases are created in another repo. @algomaster99 was the last who touched this repo. @algomaster99 how much effort is it to include one more submodule, if @monperrus gives his okay.

To properly build everything, mvn should be run from inside the spoon-pom folder and the reactor build will take over and build spoon-core and spoon-javadoc.

Is this a question, an open task or something you fixed?

@integrators: Please react with an appropriate emote if you are for this submodule.

[I-Al-Istannen]

Sorry if this was dormant for a bit

I did not have the time to push this further, so that is understandable :P

Large PRs always scare me. As this is a massive feature, I won’t merge this without the other integrators also giving green light

The original intent of this PR was not necessarily merging this into Spoon, but gathering opinions on its API first.
I will change this to WIP and first rewrite my bot when I find the time to use it. If I still like the API, we can give this another go.

A general policy for submodules would be interesting though. The alternative is keeping this around maintained by me in my own repository, which would require some kind of linktree for augmenting software for spoon.

[monperrus]

There is currently no protocol to have submodules

Good point, the solution is in the work at #4919

It seems this PR is small, simple and useful code, which would happily live in spoon-core. WDYT?

[MartinWitt]

It seems this PR is small, simple and useful code, which would happily live in spoon-core. WDYT?

I follow here slarse opinion and quote him.

Alternatively, perhaps this is something we could offer outside of "core Spoon", as some form of plugin or the like. Based on absolutely no empirical data, I'm thinking that most consumers of Spoon don't parse Javadoc. Not that we have the capacity for plugins at the moment, but perhaps we should.

This PR is massive, and Javadoc analysis seems unused. Integrating and maintaining this (forever) seems like a massive burden. There is some risk that this part of spoon consumes a lot of time and is never used. With a plugin, we have relaxed API promises and expectations from the user. Promoting this from a plugin to a core feature is easier than removing it latter.

[monperrus]

Ack, no strong opinion on my side for this PR.

[algomaster99]

Hi @MartinWitt

Releases are created in another repo. @algomaster99 was the last who touched this repo. @algomaster99 how much effort is it to include one more submodule, if @monperrus gives his okay.

The script in https://github.com/SpoonLabs/spoon-deploy does not take care of deploying modules other than spoon-core.

[I-Al-Istannen]

Okay, I rewrote my javadoc bot now using this API (resulting in https://github.com/I-Al-Istannen/DocTor and https://github.com/I-Al-Istannen/JavadocIndexer) and it was quite nice to use, saved me a lot of work and provided me with a clear structure I could mostly (de)serialize and adapt. This is extremely surprising, as it is exactly the use case I wrote it for :P Everything else would have been a disaster.

I also ironed out some kinks and I think this is mostly ready to live as a submodule now. I think I want to document somewhere that this exists, maybe in the javadoc of the current CtElement#getDocComment() and JavadocComment classes?

Once the qodana PR is merged this should be green and we should be able to go into the final stages of integrating this, I think. I am not aware of another consumer currently, though I did hear from some (n > 1) people that they would have found a proper Javadoc API useful when using spoon.

[slarse]

@I-Al-Istannen Where do we stand on this one? What's currently hindering us from integrating it as a submodule?

[I-Al-Istannen]

We could ideally find some place in the code to say "hey, this exists" as outlined in my last comment.

Apart from that I need to delete the Foo test in the main sources (and maybe add some uni tests? Not sure what we should require) and I am curious what you think about the maven pom change with the submodule layout. I think that layout forces us to also publish spoon-pom but I am not sure why the current spoon-core maven build even works with an unknown parent...

[slarse]

We could ideally find some place in the code to say "hey, this exists" as outlined in my last comment.

Oh dang, I completely missed your final comment. I will blame the fact that I'm still recovering from a solid round of covid and the flu (traded the one in for the other within a couple of days of getting better, not my best move) :D

I also ironed out some kinks and I think this is mostly ready to live as a submodule now. I think I want to document somewhere that this exists, maybe in the javadoc of the current CtElement#getDocComment() and JavadocComment classes?

That sounds like a good idea, it's where you'd go to look for it. I think in addition we could create a page in the docs, like the one that exists for spoon-smpl.

Apart from that I need to delete the Foo test in the main sources (and maybe add some uni tests? Not sure what we should require) and I am curious what you think about the maven pom change with the submodule layout. I think that layout forces us to also publish spoon-pom but I am not sure why the current spoon-core maven build even works with an unknown parent...

What is the intent of listing the modules in spoon-pom?

Horrible realization ... I think this ancient version of the spoon-pom is being used by spoon-core, I can't see how it would work otherwise, given what the docs say. For all intents and purposes, we should publish spoon-pom, I have no idea of why we don't do that. @monperrus @algomaster99 ?

It probably hasn't occurred to us since it only has test dependencies and thus shouldn't really affect production use.

[I-Al-Istannen]

Oh dang, I completely missed your final comment. I will blame the fact that I'm still recovering from a solid round of covid and the flu (traded the one in for the other within a couple of days of getting better, not my best move) :D

Ouch. That doesn't sound particularly great, get well soon :)

That sounds like a good idea, it's where you'd go to look for it. I think in addition we could create a page in the docs, like the one that exists for spoon-smpl.

I tried. I don't have a local jekyll installation, but maybe I should get one for testing. The failing extra check is also a bit annoying, I am not sure how to best resolve that…

What is the intent of listing the modules in spoon-pom?

That allows mvn package in spoom-pom to build spoon-core and spoon-javadoc in a reactor build. I can remove that, but then the workflow for building spoon-javadoc involves mvn installing the corresponding spoon-core version first. That could be solved by hardcoding a published version in spoon-javadoc, but then you have conflicting spoon-core dependencies when including spoon-javadoc. I guess that will also be a problem when using jitpack. I will need to try that one out and see if it understands building spoon-pom. If not, I will need to hardcode the spoon-core version and hope, that maven elides the correct (older) spoon-core dependency of spoon-javadoc when resolving conflicts…

Horrible realization ... I think this ancient version of the spoon-pom is being used by spoon-core, I can't see how it would work otherwise, given what the docs say.

Heh, that looks fun :P Sorald recently had a similar problem with sorald-parent, maybe there is a nice solution for both of them.

[MartinWitt]

Thanks @I-Al-Istannen and @SirYwell 🎉

[I-Al-Istannen]

It's always sad to see your baby leave for college, especially if it is just one year old :(

I hope it will live in peace, in a beautiful world with well-formed javadoc and lots of CPU cores.

---

Thanks :)

[monperrus]

the legendary one line PR merged! 🎺 🥁 🎺

thanks a lot @I-Al-Istannen

[algomaster99]

Finally, @I-Al-Istannen ! Congratulations 🎉

[slarse]

Typical one-year PR turnaround time, there was never a doubt in my mind this'd reach the finish line :D