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

Added Checkstyle plugin #3473

Closed
wants to merge 38 commits into from
Closed

Conversation

ajaychandran
Copy link
Contributor

@ajaychandran ajaychandran commented Sep 6, 2024

Resolves #3332.

The implementation differs significantly from the SBT counterpart but I feel this approach is idiomatic.

The basic functionality is ready but there is some more work (mostly documentation) to de done.
Could someone take a look and provide feedback?

@ajaychandran ajaychandran changed the title Added Checkstyle support Added Checkstyle plugin Sep 6, 2024
@ajaychandran
Copy link
Contributor Author

ajaychandran commented Sep 9, 2024

@lihaoyi
Is there a way to detect the root folder in a project? I am thinking a use-case where the Checkstyle configuration needs to be shared across modules. This is for documentation.

@ajaychandran
Copy link
Contributor Author

@lihaoyi
I botched a merge. Should I resubmit this in a new PR?

@lihaoyi
Copy link
Member

lihaoyi commented Sep 9, 2024

@ajaychandran whatever you prefer. You can also make a new branch locally force push it to the existing PR branch on github

@ajaychandran
Copy link
Contributor Author

@lihaoyi
A usecase like a CI build would require the build to fail if Checkstyle errors were found.
I will add a task that checks the error count. Is there a naming convention for such tasks?

@lihaoyi
Copy link
Member

lihaoyi commented Sep 10, 2024

@ajaychandran we don't have a convention unfortunately; the current names we have used are

./mill -i mill.scalalib.scalafmt.ScalafmtModule/checkFormatAll __.sources + __.mimaReportBinaryIssues + __.fix --check

checkCheckStyle would be a bit weird. I'd say come up with a name that makes sense yourself and we can normalize all these things later

@ajaychandran
Copy link
Contributor Author

ajaychandran commented Sep 10, 2024

@lihaoyi Please review.

Notable changes:

  • checkstyle is now a command that accepts --check and --stdout flags. It also accepts a list of files/folders to be processed.
  • The exit code byte conversion was removed.

One issue is that the --stdout flag is not working.
The output report does not show up on the console. The program only prints the number of violations.

@ajaychandran
Copy link
Contributor Author

@lihaoyi
I think the original design with 2 plugins was cleaner.
If you have no objections, I will close this PR and submit a new one.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 10, 2024

go for it

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.

Add Java Checkstyle Plugin (1000USD Bounty)
2 participants