-
Notifications
You must be signed in to change notification settings - Fork 3k
introduce AvoidStarImport rule via maven-checkstyle-plugin
#46160
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 AvoidStarImport rule via maven-checkstyle-plugin
#46160
Conversation
da16c88 to
4e86afd
Compare
| <module name="TreeWalker"> | ||
| <module name="AvoidStarImport"/> | ||
| </module> | ||
| <!-- <module name="LineLength">--> |
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 needs to be discussed and either removed or addressed step by step, as it is currently not compliant.
4e86afd to
1e57060
Compare
.editorconfig
Outdated
| end_of_line = lf | ||
| indent_style = space | ||
| insert_final_newline = false | ||
| max_line_length = 120 |
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 of course needs to be adjust to your needs as idk the defaults here around i suggested the imho most common.
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.
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.
thx
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.
Reconsidering this, IMHO, it only makes sense again with the corresponding editorconfig-maven-plugin, as this ensures that the theory (config) is applied in reality (code) as seen here: https://github.com/urlaubsverwaltung/urlaubsverwaltung/blob/5350de17c6ba9b0b1d60e983bb2642b479c5d8bb/pom.xml#L418
Although this is only meant to be an addition to the current Eclipse formatter, as this lacks the end_of_line = lf config, or I don’t see it.
Just blocking out the star imports would be compatible and minimally invasive, only causing attention in a really avoidable situation.
|
I like the |
|
Thanks for the feedback. The goal was to introduce a seamless, out-of-the-box solution for coding guidelines, reducing manual code formatting evaluation, which is time-consuming and error-prone.
Checkstyle is opinionated but only enforces configured rules. Currently, we only enforce one rule: forbidden wildcard imports. A hybrid solution could enforce critical rules with Checkstyle while leaving other aspects untouched, reducing manual effort. |
1e57060 to
6942fa8
Compare
6942fa8 to
a03e5db
Compare
wildcard * import via maven-checkstyle-pluginAvoidStarImport rule via maven-checkstyle-plugin
|
We know that ChatGPT is useful, but please refrain from using it to explain every change you did, that's a bit annoying |
|
yes please excuse this, as im having some kind of difficulties with the language and syntax. I will try to imporve. |
|
Sorry, introducing Checkstyle requires an ADR and previous discussion, we're closing this PR for now. |
|
ADR: #46247 |
|
@gastaldi i think we talked about using rewrite: FYI: consider licence. what do you say about it as you have it involved? |
prerequisite: #46149
resolve: #45898
apply: https://checkstyle.sourceforge.io/checks/imports/avoidstarimport.html#Example1-config
This change would address the issue of wildcard imports and reduce the effort for new developers, as well as for those (like myself) who need to reference the documentation before committing. It would help reduce noise and friction, making things work out of the box and more self-explanatory.
@rhusar @gastaldi