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

Remove blank Javadoc #1566

Merged
merged 1 commit into from
Nov 8, 2023
Merged

Remove blank Javadoc #1566

merged 1 commit into from
Nov 8, 2023

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Nov 7, 2023

Remove blank Javadoc

This commit cleans up Javadoc that does not add information.
It resolves ecj warnings:
Javadoc: Description expected after ...
It helps to prevent future empty javadoc by disabling
missingJavaDoc warnings. This resolves
Javadoc: Missing ...

The modification is a result of regular expression search&replace:

in files *.java (exclude org.eclipse.jdt.core.tests.model/workspace)
^[\s]*\*[\s]*(@return|@param[\s]*[^\s]+|@throws[\s]*[^\s]+)\R([\s]*\*[\s]*@|[\s]*\*/\R)
->$2
^([\s]*\*[\s]*\R)([\s]*\*/\R)
->$2
^[\S\t]*/\*\*\R[\s]*\*/\R
->``

in files org.eclipse.jdt.core.prefs
org\.eclipse\.jdt\.core\.compiler\.problem\.missingJavadoc(Comments|Tags)\=[^\s]*
->org\.eclipse\.jdt\.core\.compiler\.problem\.missingJavadoc$1\=ignore

This commit cleans up Javadoc that does not add information.
It resolves ecj warnings:
 `Javadoc: Description expected after ...`
It helps to prevent future empty javadoc by disabling
missingJavaDoc warnings. This resolves
 `Javadoc: Missing ...`

The modification is a result of regular expression search&replace:

in files `*.java` (exclude org.eclipse.jdt.core.tests.model/workspace)
 `^[\s]*\*[\s]*(@return|@param[\s]*[^\s]+|@throws[\s]*[^\s]+)\R([\s]*\*[\s]*@|[\s]*\*/\R)`
 ->`$2`
 `^([\s]*\*[\s]*\R)([\s]*\*/\R)`
 ->`$2`
 `^[\S\t]*/\*\*\R[\s]*\*/\R`
 ->``

in files `org.eclipse.jdt.core.prefs`
 `org\.eclipse\.jdt\.core\.compiler\.problem\.missingJavadoc(Comments|Tags)\=[^\s]*`
 ->`org\.eclipse\.jdt\.core\.compiler\.problem\.missingJavadoc$1\=ignore`
@jukzi jukzi merged commit 1b0577c into eclipse-jdt:master Nov 8, 2023
8 of 9 checks passed
@jukzi jukzi deleted the blankJavaDoc branch November 8, 2023 07:15
@stephan-herrmann
Copy link
Contributor

I could use some help to understand what exactly was improved here.

  • Per JDT's project preferences most of these warnings are ignored in the IDE, are the same settings also used during tycho builds?
    • As I don't see any such warnings, what do you mean by "This resolves ..."
    • The issue descriptions speaks of editing files org.eclipse.jdt.core.prefs but I can't find any such change
  • What is improved by replacing one bad (tag without a description) with another bad (missing tag)?
  • What is improved by removing empty comment lines (* followed by nothing)?

@jukzi what do you expect JDT committers to do to ensure "better" javadoc in the future? Do we have agreement about "better" in the first place. E.g., I don't see a need to enforce any of this in internal packages, where I'm happy about every bit of documentation I find, even if some bits are missing. So, what is the long-term goal?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something went wrong on this class!
Hint: you broke all line endings.
It seems this mass change was not quite as safe as assumed.

Copy link
Contributor Author

@jukzi jukzi Nov 16, 2023

Choose a reason for hiding this comment

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

I don't see i broke or even changed any line ending in this file:
image
this is screenshot from windows, using autocrlf. If something changed it may be a egit or jgit problem or the line endings might have been wrong before

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course it works on your machine :) but that's not the same as what you changed in git.
Please look at the change in github: it says for this file specifically: "Large diffs are not rendered by default."
Also the header of this file seems to say that you changed 32,528 lines. In cause of doubt, git is "the truth".

or the line endings might have been wrong before

"wrong" is a relative term, but yes, the file dangerously had mixed line endings before, since a recent commit added one \r\n. But now all are windows line ends in git, where it should be unix style.

Copy link
Contributor

Choose a reason for hiding this comment

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

Being the person who introduced the original \r\n inadvertently I worry it is going to happen again. I copy + paste between windows and Linux VM frequently and this problem may recur since the tool (Vim) may be going out of the way to preserve line endings. Shouldn't eclipse have a setting or save action that strips away the \r ??

@akurtakov
Copy link
Contributor

@stephan-herrmann This comment is not related to this PR and its effects!
FYI: There is an effort to remove one layer of indirection in building javadocs (eclipse-platform/eclipse.platform.releng.aggregator#1531) . Over the years releng side tried to get this more and more straightforward but having javadoc errors visible only after the I-build is clearly non-scalable solution thus we want verification builds to fail on javadoc errors too. The way javadoc is built right now is in the most loose way as can be seen by the amount of errors javadoc gives at https://ci.eclipse.org/platform/job/eclipse.platform.releng.aggregator/job/PR-1531/5/console . While I don't see javadoc being logically enhanced there are possible misrendered informations and overall it is step first (of many) to strenghten the requirements around documentation.

@stephan-herrmann
Copy link
Contributor

@akurtakov I'm all in favor of improving javadoc quality. Is the overall effort directed at all bundles, or only doc-bundles?

Do we have a ticket to check if JDT-validations - when enabled - would detect all those errors that currently only surface when generating javadoc? This could help to ensure we are actually fixing the right problems.

@akurtakov
Copy link
Contributor

@akurtakov I'm all in favor of improving javadoc quality. Is the overall effort directed at all bundles, or only doc-bundles?

Do we have a ticket to check if JDT-validations - when enabled - would detect all those errors that currently only surface when generating javadoc? This could help to ensure we are actually fixing the right problems.

Not that I know of, my current goal here is to have JDT doc bundles from https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/tree/master/eclipse.platform.common/bundles to https://github.com/eclipse-jdt/eclipse.jdt . That would allow JDT committers that are not Platform committers to be able to maintain docs without waiting for Platform committer to push.
Improving JDT validations would be more than welcome and first and supposedly not very hard thing should be to detect non-escaped &, <, >... symbols which I think JDT don't have support to warn about.

@jukzi
Copy link
Contributor Author

jukzi commented Nov 16, 2023

in files org.eclipse.jdt.core.prefs
org\.eclipse\.jdt\.core\.compiler\.problem\.missingJavadoc(Comments|Tags)\=[^\s]*
->org\.eclipse\.jdt\.core\.compiler\.problem\.missingJavadoc$1\=ignore

@stephan-herrmann i went throughout all repositories in platform workspace. Actually for this repository no preferences where changed. A bad tag is an error while a missing tag is not / depends on preferences. Both empty tags and empty lines are boiler plate. Typically that boiler plate was added with a quick fix to add missing tags, without actually documenting the parameters which does not help at all.

@stephan-herrmann
Copy link
Contributor

@stephan-herrmann i went throughout all repositories in platform workspace.

Is that why we don't have an issue filed in JDT?

Actually for this repository no preferences where changed.

And your PR description was just the same for all repos, irrespective of actual changes, right?

A bad tag is an error while a missing tag is not / depends on preferences.

Where do you see this difference? JDT can report missing tags and missing descriptions. It seems that in all our projects both problems are configured to ignore.

This means that future editing will run the same risk of creating meaningless javadoc.

Both empty tags and empty lines are boiler plate. Typically that boiler plate was added with a quick fix to add missing tags, without actually documenting the parameters which does not help at all.

I didn't say that empty tags are good, just that no tag is not better.

So, what was the real goal of this change? From Alex's links I seem to understand you started by fighting problems causing the javadoc tool to fail. Which are they exactly? But it seems you ended up changing also things which don't matter for the tool?

After asking all kinds of questions, these are my concerns:

  • IMHO the change is far to big to go under the radar of filing an issue and requesting a review.
  • For the part that causes the javadoc tool to fail, I'd expect that we first try to identify the problems using JDT's own analysis, which would mean every PR build would fail if new problems are introduced. This could mean we put priority on improving this analysis in the first place. That's called dog-fooding :)
  • For a mass change I prefer that one problem is addressed at a time. Empty lines are certainly not of the kind to break the javadoc tool.

@jukzi
Copy link
Contributor Author

jukzi commented Nov 17, 2023 via email

@jukzi
Copy link
Contributor Author

jukzi commented Nov 17, 2023 via email

@stephan-herrmann
Copy link
Contributor

Maybe we could add .gitattributes to force unix style on .java files. https://bugs.eclipse.org/bugs/show_bug.cgi?id=342372 ?

Sounds good to me, but as I'm on linux I don't have experience with \r\n inadvertently entering the picture during editing / pasting. Anyone have experience with such .gitattributes on windows?

Also we should first agree on what to do with the existing files that do contain one or more windows line ending.

(git/eclipse.jdt.core) $ find . -name \*.java -exec /bin/grep -qzoP "\r\n" {} \; -print |wc -l
127

Interestingly, of those 127 files, 120 reside in JCL, so perhaps we'd just exclude that folder from this round of clean-up? Since those files are just test stubs, not productive code I could be persuaded either way.

Here's the list of files I suggest to fix in one go:

org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/SwitchPatternTest.java
org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/Deprecated18Test.java
org.eclipse.jdt.core.tests.model/workspace/JavaSearchBugs/src/b573388/C.java
org.eclipse.jdt.core.tests.model/workspace/JavaSearchBugs/src/b573388/R.java
org.eclipse.jdt.core.tests.model/workspace/JavaSearchBugs/src/b573388/X.java
org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/formatter/FormatterRegressionTests.java
org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/model/ResolveTests12To15.java

@jukzi
Copy link
Contributor Author

jukzi commented Nov 23, 2023

@msohn is there any best-practice to avoid mixed line endings checked in for .java files?

@msohn
Copy link

msohn commented Nov 23, 2023

https://www.aleksandrhovhannisyan.com/blog/crlf-vs-lf-normalizing-line-endings-in-git/

  • In the repository text files should always be stored using LF.
  • If possible avoid mixed line endings in text files.
  • The checked out files in the working tree can use LF or CRLF (Windows).
  • Git can do automatic conversion from LF stored in the repository to the desired line ending, depending on the OS, during git checkout (done by the so called smudge filter) and vice versa the reverse conversion on git add (so called clean filter).
  • Disadvantage of this automatic conversion is a performance penalty (on Windows) since conversions have to be done on every checkout/add for files handled by these operations.
  • If you in addition use LFS for storing large files in another storage (e.g. S3) these conversions are even stacked since on checkout the LFS smudge filter first replaces the LFS pointer file stored in the repository by the actual blob stored in the LFS backend and then the git line-ending smudge filter converts line endings so that the files in the working tree have the desired line endings. On git add the reverse conversions (convert line ending back to LF, send resulting file to LFS backend, store updated pointer file in git) are done in the reverse direction.
  • If you need CRLF line endings on Windows I think the best way for handling these OS dependent conversions is using .gitattributes as described in the linked article.

@jukzi
Copy link
Contributor Author

jukzi commented Nov 24, 2023

  • In the repository text files should always be stored using LF.

All the other recommendations only target how files are checked out. Is there no way to force conversion to LF on check in or rejecting CRLF?

@iloveeclipse
Copy link
Member

You can batch convert CRLF to LF on Windows with File -> Convert Line Delimiters -> Linux, if that was the question.

"On save" you can enable AnyEdit to do that, assuming your workspace / project is set to LF by default.

image

@jukzi
Copy link
Contributor Author

jukzi commented Nov 24, 2023

The question is if commits can be automatically rejected or autofixed if somebody does it wrong.

@iloveeclipse
Copy link
Member

That's what git would do automatically, once configured, as explained by Matthias above.

@jukzi
Copy link
Contributor Author

jukzi commented Nov 24, 2023

that only affects git checkout

@iloveeclipse
Copy link
Member

iloveeclipse commented Nov 24, 2023

that only affects git checkout

Nope, see

and vice versa the reverse conversion on git add (so called clean filter).

@msohn
Copy link

msohn commented Nov 24, 2023

You can check line endings in the repo using $ git ls-files --eol. It displays line endings found for each file, i/ for index and repo, w/ for files in the working tree and attr/ which attribute in .gitattributes applies to the file.

If you would still use Gerrit (;-)) you could use its uploadvalidator plugin which allows to reject pushes containing text files using Windows line endings [1].

You could setup a CI job which uses the output of git ls-files --eol to reject files which use unwanted line endings.

[1] https://gerrit.googlesource.com/plugins/uploadvalidator/+/refs/heads/master/src/main/resources/Documentation/config.md

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.

7 participants