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

Adding linter #163

Draft
wants to merge 20 commits into
base: v3_FileInfoImprovementAndRequestHandler
Choose a base branch
from
Draft

Conversation

LewerenzM
Copy link
Contributor

@LewerenzM LewerenzM commented Jun 5, 2024

Trying out some approaches for linting the code.

  • First tried prettier-maven-plugin with it's simple configuration. But it lacks an import ordering feature. That feature is an extra plugin for prettier and not yet realized for maven (seems so to me).
  • Tried the (eclipse) formatter-maven-plugin as next, but it also has no possibility for ordering imports
  • The spotless-maven-plugin unites the eclipse formatter and the eclipse import ordering feature. So this seems the best approach for now.

Like described in the README.md, the local build will automatically change the code to fulfil the guidelines. The CI build will only make a check and fails if the code style violates the defined guidelines.

@LewerenzM LewerenzM self-assigned this Jun 6, 2024
@LewerenzM LewerenzM changed the title Added prettier maven plugin Adding linter Jun 12, 2024
@LewerenzM
Copy link
Contributor Author

The code is not yet linted, because we should speak about the specific guidelines we'd like to define. That's why the CI build fails, currently.

@RKrahl
Copy link
Member

RKrahl commented Jul 31, 2024

Many thanks for submitting this! Although I'm not able to formally start a review as long as the PR is marked as draft, I might just add a preliminary one as a comment here:

First of all, generally, this looks good to me. There are a few minor remarks, though:

  • Obviously, as you noted yourself in your last comment, we need to discuss the style guidelines being imposed. And we definitely should include @GonozalIX in that discussion.
  • If this is supported by the spotless plugin, we might consider to downgrade a few of the guidelines to warnings, e.g. spotless should emit a warning for a violation of the particular rule, but the CI should not fail on this and a local run should not modify the file. This seems particularly relevant for any limitation of the length of code lines.
  • You created this PR based on the v3_FileInfoImprovementAndRequestHandler branch and thus it implicitly depends on V3 file info improvement and request handler #157. There is no reason for this dependency. In fact, there is no reason for this change to wait for version 3. It could go into master right away instead. (Since you can't change the source branch of a PR in GitHub, this would require to create a new PR, though.)
  • The config files eclipse-formatter.xml and eclipse-importorder.txt are cluttering the main directory. I would prefer them to be stashed away in a subdirectory like .spotless.

@RKrahl RKrahl mentioned this pull request Aug 15, 2024
2 tasks
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.

2 participants