-
Notifications
You must be signed in to change notification settings - Fork 36
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
Introduce {CONTRIBUTING,LICENSE,README}.md
and Error Prone Support's logo
#212
Conversation
error-prone-contrib/README.md
Outdated
When loading the project in IntelliJ IDEA (and perhaps other IDEs) errors about | ||
the inaccessibility of `com.sun.tools.javac.*` classes may be reported. If this | ||
happens, configure your IDE to enable the `add-exports` profile. | ||
See [README.md][main-readme]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below is duplicate now, therefore linking to the new location. The rest of this content's page should find a better home, but I considered that outside the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drive by comment: We should put a medium complexity but common before-vs-after use case at the top of the readme so that it's immediately visible what transformations this software enables.
Indeed. The plan was to add usage samples and check samples (to get started for contributions) in a follow-up PR. However, suggestions on this PR are welcome. |
fe2e885
to
ebd7cf0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first pass, this looks really cool man. Thanks for all the effort and working on these changes! Love the logo ❤️:smile:!
README.md
Outdated
<picture> | ||
<source media="(prefers-color-scheme: dark)" srcset="logo-dark.svg"> | ||
<source media="(prefers-color-scheme: light)" srcset="logo.svg"> | ||
<img alt="Error Prone Support logo'" src="logo.svg" width="50%"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<img alt="Error Prone Support logo'" src="logo.svg" width="50%"> | |
<img alt="Error Prone Support logo" src="logo.svg" width="50%"> |
README.md
Outdated
[google-java-format]: https://github.com/google/google-java-format | ||
[licence-badge]: | ||
https://img.shields.io/github/license/PicnicSupermarket/error-prone-support | ||
[licence]: LICENSE.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[licence]: LICENSE.md | |
[license]: LICENSE.md |
README.md
Outdated
Error Prone Support is a Picnic-opinionated extension of [Error | ||
Prone][error-prone-repo] to improve code quality and maintainability. | ||
|
||
> Error Prone is a static analysis tool for Java that catches common programming |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to show, in a way, that this is a quote from errorprone.info
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I linked @google/error-prone above (as [error-prone-repo]
), which is also where this comes from. Would be enough, I think :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a commit.
Shall we also enable Github Discussions? If so, we should reference it somewhere.
Didn't approve yet because I might want to do another pass later.
README.md
Outdated
[error-prone-installation-guide]. Next, edit your `pom.xml` file to add Error | ||
Prone Support to your project. See [@PicnicSupermarket/oss-parent/pom.xml] | ||
[oss-parent-example] for an example configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oss-parent
does not yet use Error Prone Support. We should either fix that (preferred) or drop this.
Edit: let's move it to an XXX
comment.
README.md
Outdated
- `-Dverification.warn` makes the warnings and errors emitted by various plugins | ||
and the Java compiler non-fatal, where possible. | ||
- `-Dverification.skip` disables various non-essential plugins and compiles the | ||
code with minimal checks (i.e. without linting, Error Prone checks, etc.) | ||
- `-Dversion.error-prone=some-version` runs the build using the specified | ||
version of Error Prone. This is useful e.g. when testing a locally built Error | ||
Prone SNAPSHOT. | ||
- `-Perror-prone-fork` run the build using Picnic's [Error Prone | ||
fork][error-prone-fork-repo], hosted on [Jitpack][error-prone-fork-jitpack]. | ||
This fork generally contains a few changes on top of the latest Error Prone | ||
release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also document -Pself-check
. (Right now that profile is compatible only with the fork, as it relies on -XepAllSuggestionsAsWarnings
.)
README.md
Outdated
<!-- Error Prone Support's additional bug checkers. --> | ||
<path> | ||
<groupId>tech.picnic.error-prone-support</groupId> | ||
<artifactId>error-prone-contrib</artifactId> | ||
<version>${error-prone-support.version}</version> | ||
</path> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shows that error-prone-contrib
is not a great name. Perhaps we should split this module into more topical smaller modules before open-sourcing 🤔.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss this one also in the light of the TestNG migration checks.
sneak in unrelated changes. | ||
- When in doubt about whether a pull request will be accepted, please first | ||
file an issue to discuss it. | ||
See [CONTRIBUTING.md][main-contributing]. | ||
|
||
### Our wishlist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list should definitely be curated before go-live.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move this list to bugcheck-ideas.md
and refaster-ideas.md
. Let's do in a follow-up.
42910b0
to
51680a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative suggested commit message:
Introduce {CONTRIBUTING,LICENSE,README}.md and Error Prone Support's logo
Thanks for driving this forward @japborst 🚀 !
I've added a small bugcheck/refaster usage example. Would appreciate a final look @rickie. Any remaining feedback @Stephan202? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example makes it even nicer, can't wait to see this merged 🚀 !
README.md
Outdated
[WARNING] COMPILATION WARNING : | ||
[INFO] ------------------------------------------------------------- | ||
[WARNING] Example.java:[9,34] [tech.picnic.errorprone.refastertemplates.BigDecimalTemplates.BigDecimalZero] | ||
null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Stephan202 this is what we talked about a little while ago, it shows null
. We could leave it out here for now or leave it in, fine with both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it; optimistically assuming this will be solved at some point 😉
[WARNING] Example.java:[9,34] [tech.picnic.errorprone.refastertemplates.BigDecimalTemplates.BigDecimalZero] | ||
null | ||
Did you mean 'return BigDecimal.ZERO;'? | ||
[WARNING] Example.java:[14,35] [IdentityConversion] This method invocation appears redundant; remove it or suppress this warning and add a comment explaining its purpose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@japborst and I talked about it and updated the example. This nicely shows the two SuggestedFix
es, which is cool 😄.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very important and impactful review
CONTRIBUTING.md
Outdated
on GitHub. | ||
|
||
Before doing so, please: | ||
- Verify that issue is reproducible against the latest version of the project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify that issue is reproducible against the latest version of the project. | |
- Verify that the issue is reproducible against the latest version of the project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Still needs to be applied 😄 (Will do.)
08a9122
to
bbcddb6
Compare
Rebased and resolved conflicts. |
CONTRIBUTING.md
Outdated
on GitHub. | ||
|
||
Before doing so, please: | ||
- Verify that issue is reproducible against the latest version of the project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ Still needs to be applied 😄 (Will do.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review. Have a meeting now; more later. I like how this shapes up!
error-prone-contrib/README.md
Outdated
[checkstyle-external-project-tests]: | ||
https://github.com/checkstyle/checkstyle/blob/master/wercker.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that line-wrapping would have this spread across two lines, but this makes it harder to keep the entries sorted and check whether the keys are "consistent". If/when we introduce Prettier (or similar) to format the file this might be inevitable, but for now let's keep these on a single line. (Like we still do in e.g. CONTRIBUTING.md
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was indeed formatted using prettier
. Not applying line-length here seems a bit "meten met twee maten" (double standards), but alas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I suspect I'll come around soon enough 🙈
Thanks @Stephan202. Agree with the suggestions 👍 |
03190f9
to
603401f
Compare
Rebased and resolved conflict. |
603401f
to
a7efb92
Compare
a7efb92
to
1b4e7d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased and added two more commits with assorted tweaks. Sorry for the delay; happy that we pulled this off!
Suggested commit message:
Introduce `{CONTRIBUTING,LICENSE,README}.md` and Error Prone Support's logo (#212)
[modernizer-maven-plugin]: https://github.com/gaul/modernizer-maven-plugin | ||
[pitest]: https://pitest.org | ||
[pitest-maven]: https://pitest.org/quickstart/maven | ||
[sonarcloud]: https://sonarcloud.io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👇 I tried to minify the SVG files using this website, but then the cute bug at the bottom is gone 😅
(But if you know a better way to reduce file size, that'd be nice.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that gzipped, it's already 26kb instead of 110kb.
I optimized them a bit using https://jakearchibald.github.io/svgomg/ (from 100 to ~65kb). When gzipped this is barely an improvement, you go from 26 to 24kb, but alas.
@ferdinand-swoboda any other feedback? 😄 @rickie @japborst if you're okay with my latest changes then I think this one is good to go :) |
A |
Nice improvements! They look good to me :). |
Changes LGTM. Also verified that all the anchors work. Let's go 🚀 |
{CONTRIBUTING,LICENSE,README}.md
and Error Prone Support's logo
Suggested commit message: