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

Inconsistent checkstyle rules in the project itself #351

Closed
Bananeweizen opened this issue May 29, 2022 · 1 comment
Closed

Inconsistent checkstyle rules in the project itself #351

Bananeweizen opened this issue May 29, 2022 · 1 comment

Comments

@Bananeweizen
Copy link
Collaborator

The Eclipse plugins are currently configured to use the Google checks, see for instance https://github.com/checkstyle/eclipse-cs/blob/master/net.sf.eclipsecs.core/.checkstyle. However, during a PR build, the (otherwise not referenced) configuration from https://github.com/checkstyle/eclipse-cs/blob/master/config/checkstyle_checks.xml is used. That leads to the local IDE being free of errors, but pull requests failing, as I experienced myself.

I could create a project relative configuration to use that shared configuration file, but there are multiple issues:

  • It has 7 properties and those would need to be defined to make EclipseCS be able to use it. Those properties must be set during the build somehow, does anyone know where and how?
  • Using this project relative configuration requires that every EclipseCS developer imports the root level Maven project into the Eclipse workspace. Unfortunately it is not possible to specify a relative configuration location without a containing project.
  • That shared config file looks like a copy of the file from the main Checkstyle project. It should probably be updated, or be referenced from there directly. Similar to the previous item, this would require every EclipseCS developer to have the root project of the Checkstyle repo be imported in the Eclipse workspace.

Once this configuration has been updated, a lot of errors need to be expected. Those should be fixed before doing any further Java work. Otherwise every PR has the chance of failing with Checkstyle errors as happened here: #350

@rnveach
Copy link
Member

rnveach commented May 29, 2022

IMO, I would start by updating https://github.com/checkstyle/eclipse-cs/blob/master/config/checkstyle_checks.xml to be the configuration you wish to be use for all of eclipse-cs. It is already controlled by CI, so the only thing left would be to configure Eclipse to use it.

I would be willing to assist with violations once this configuration is defined.

For the main repository, we have a few sub-repos that also use the main's configuration file so they all use the same configuration. I don't know eclipse-cs/Eclipse well enough if it could make use of it, but we define it in maven via the following items:

Specify the external configuration location in the POM:
https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/pom.xml#L22
Values to the repo-specific properties used inside the config:
https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/checkstyle.properties

It has 7 properties and those would need to be defined to make EclipseCS be able to use it. Those properties must be set during the build somehow, does anyone know where and how?

I don't know which 7 properties being referred to, but if it is similar to the property file I mentioned above, I would think there should be some way for eclipse-cs/Eclipse to specify the file or something similar. The file itself may not be part of the main checkstyle API, but checkstyle allows to specify property values on the command line to be pulled in.
See:
https://checkstyle.org/config.html#Properties
https://checkstyle.org/cmdline.html#Command_line_usage

-D<property>=<value>

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

No branches or pull requests

2 participants