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

[MJAVADOC-784] Upgrade to Doxia 2.0.0 Milestone Stack #204

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

michael-o
Copy link
Member

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MJAVADOC-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MJAVADOC-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify -Prun-its to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@michael-o
Copy link
Member Author

michael-o commented Nov 19, 2023

@kriegaex Have a look at the second commit. It is a prototype how to align this plugin with the behavior of Maven Reporting Impl.

@asfgit asfgit force-pushed the doxia-2.0.0 branch 5 times, most recently from f877d10 to 99d4d64 Compare November 19, 2023 20:44
@kriegaex
Copy link

kriegaex commented Nov 20, 2023

@michael-o, I am quite busy and only took a quick glance at the changes. But to me it seems as if before, you could override the default directory including the "apidocs" part. Now, it looks as if you can only adjust the report base directory, but "apidocs" is hard-coded. I would have expected that part to still be configurable. Existing projects which store the API docs in, say target/javadoc for direct mojo execution or target/site/javadoc for site generation, would no longer have a way to configure that.

If my assumption is wrong, it is of course my fault 100%, because I spent no more than 2 minutes, scrolling through the changes, which certainly by no means qualifies as a proper code review.

@michael-o
Copy link
Member Author

@michael-o, I am quite busy and only took a quick glance at the changes. But to me it seems as if before, you could override the default directory including the "apidocs" part. Now, it looks as if you can only adjust the report base directory, but "apidocs" is hard-coded. I would have expected that part to still be configurable. Existing projects which store the API docs in, say target/javadoc for direct mojo execution or target/site/javadoc for site generation, would no longer have a way to configure that.

If my assumption is wrong, it is of course my fault 100%, because I spent no more than 2 minutes, scrolling through the changes, which certainly by no means qualifies as a proper code review.

Your observations are correct, let me clarify:

(1) We know the semantical behavior of outputDirectory in AbstractMavenReport now regardless of the actual value. This plugin should use the same semantical behavior.
(2) All other plugins with subdirs have fixed names, you can only control the output directory, everything else is at the discretion of the plugin
(3) I have removed it on purpose right now because it wanted it to work with less complexity. I wonder whether there is a real world usecase to change apidocs to something else...though I don't see a problem to introduce a reportOutputDirName or similar which is really a name an not the same as the distDir as before.

WDYT?

@kriegaex
Copy link

kriegaex commented Nov 22, 2023

All other plugins with subdirs have fixed names, you can only control the output directory

That might be so. I neither know nor use all plugins. But at least, for Javadoc and JXR it currently is possible to change that. Generally, I agree that most people will just use defaults for the destination directories (destDirs, I mean the last part of the final directory path), but we have no way of knowing, because in many cases they do not have a choice. I also find it unfortunate that the destDir names are fixed, and many of them I do not like. On the one hand, one could argue that this is just cosmetics. On the other hand, IMO it is suboptimal that a bunch of Maven plugins feel entitled to just usurp those directory "namespaces". For instance, I would prefer "javadocs" to "apidocs", "unit-test-reports" to "surefire-reports" and "integration-test-reports" to "failsafe-reports".

Having said that, I acknowledge that the current state of affairs, though suboptimal, is certainly something I can live with. Arguably, it might be out of scope of this PR to change it.

everything else is at the discretion of the plugin (...) I wonder whether there is a real world usecase to change apidocs to something else

Yes, other plugins outside the set of Maven core plugins offer the option to change destDirs, and I rather like it. One such example is the plugin I was working one when we got this whole avalanche here rolling: AspectJ Maven Plugin. Actually, the AspectJ report first creates normal javadocs and then decorates them with additional information about which aspects affect which classes/methods and vice versa. In this regard, the AspectJ Maven report replaces the Maven Javadoc report, i.e. some users might like the option to change the default destDir "aspectj-report" to "apidocs" to further document that fact. They can do that, because the plugin permits them to do it. If for whatever reason the user also redundantly wants to generate the original javadocs, in the future he could not rename "apidocs" to "apidocs-orig" or whatever. That is by no means a big deal, I just meant to provide an example.

@kriegaex
Copy link

LGTM.

First, I opened c79769a and was confused after you just merged apache/maven-reporting-impl#26, seeing lots of mojo executions under target/site/apidocs. Then I noticed ba931bc, which changed the previous commit again, now sporting target/reports/apidocs, but has a somewhat anonymous commit message "MSHARED". Probably, you are going to create a Jira issue, edit the commit message and force-push the last commit once more, if I am guessing correctly.

@michael-o
Copy link
Member Author

LGTM.

First, I opened c79769a and was confused after you just merged apache/maven-reporting-impl#26, seeing lots of mojo executions under target/site/apidocs. Then I noticed ba931bc, which changed the previous commit again, now sporting target/reports/apidocs, but has a somewhat anonymous commit message "MSHARED". Probably, you are going to create a Jira issue, edit the commit message and force-push the last commit once more, if I am guessing correctly.

That's WIP, the actual code works. The reason you see both reports/ and site/ is that some ITs use standalone invocation and others site invocation. That's actually a good sign that the code works as expected after the split.

@kriegaex
Copy link

The reason you see both reports/ and site/ is that some ITs use standalone invocation and others site invocation.

I know that. You misread my comment. But before your force-push, there was only site, and I reviewed the old commit separately first. Only later, I noticed the new commit, which now takes reports into account, too. This is why I wrote "LTGM" (looks good to me) on top of my post.

@asfgit asfgit force-pushed the doxia-2.0.0 branch 3 times, most recently from 21721ee to ac8986b Compare December 16, 2023 13:47
@michael-o michael-o changed the title Prepare for Doxia 2.0.0 [MJAVADOC-784] Upgrade to Doxia 2.0.0 Milestone Stack Jan 1, 2024
asfgit pushed a commit that referenced this pull request Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants