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

Basic CheckStyle validation #3781

Merged
merged 11 commits into from Dec 12, 2022

Conversation

4everTheOne
Copy link
Contributor

@4everTheOne 4everTheOne commented Dec 10, 2022

This PR pretends to help improving the quality of the code, by providing a code style validation while the feature is in development.

This is a simple CheckStyle based at Coding Guidelines.

What do you think of this feature? Would it help delivering value to the project?

@4everTheOne 4everTheOne changed the title Project basic CheckStyle validation Basic CheckStyle validation Dec 10, 2022
@jlerbsc
Copy link
Collaborator

jlerbsc commented Dec 10, 2022

There are 2419 errors reported by Checkstyle 8.45.1 with dev-files/JavaParser-CheckStyle.xml ruleset. The approach is good but it is unmanageable.

@4everTheOne
Copy link
Contributor Author

4everTheOne commented Dec 10, 2022

Hi @jlerbsc , thanks for your feedback. I totally agree with you when you say it's unmanageable.
I think we can improve the approach of implementation by gradually enforcing the rules.

Breaking down the 2419 results we have the following cases:

  • LineLength: 1628 (The line length exceeds 120 characters per line)
  • Indentation: 292 (Code is wrongly indented)
  • CustomImportOrder: 499 (Importers are placed in the wrong order)

One way of doing this, would be to changing the severity of "LineLength" from error (which means mandatory) to a warning. By doing this, it will not break the build but for developers will have a warning notifying about a improvement to be done, like shown below:
image
Later, when in a more manageable stage we can revert it back to mandatory :)

About the topic Indentation and CustomImportOrder, this can be easily fixed with the IDE tools and if you want i can include that fix on another PR, or if you prefer on this one :)

Do you think it can be manageable this way?

@jlerbsc
Copy link
Collaborator

jlerbsc commented Dec 11, 2022

It actually seems a lot more manageable to me. But above all, it must not block a PR for the moment.

@4everTheOne
Copy link
Contributor Author

Updated the Checkstyle to report every violation as Warning.
When we think is time to enforce, we call change it back to error

@jlerbsc jlerbsc merged commit b3b818a into javaparser:master Dec 12, 2022
@jlerbsc jlerbsc added this to the Next release milestone Dec 12, 2022
@jlerbsc jlerbsc added the PR: Development A PR that affects only JavaParser developers (e.g. build / deploy infrastructure) label Dec 12, 2022
@4everTheOne 4everTheOne deleted the improvement/checkstyle branch December 12, 2022 23:06
@4everTheOne 4everTheOne mentioned this pull request Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Development A PR that affects only JavaParser developers (e.g. build / deploy infrastructure)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants