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

Add extra checkstyle checks in an IDE #67650

Merged
merged 4 commits into from
Jan 21, 2021

Conversation

pugnascotia
Copy link
Contributor

Now that Checkstyle can be made useful in an IDE, add extra rules only when checking in the IDE so that a contributor is given extra help when editing files, without having the checkstyle task spam the console when running gradle.

Apart from the BooleanNegation rule below, the new rules have the warning severity level. The new Javadoc rules reflect the guidelines in CONTRIBUTING.md that we've had for some time.

  • I upgraded Checkstyle, so now it interprets the config file in the same was as the IntelliJ plugin. That means that I could move the LineLength rule up a level and remove its special handling in the :configureIdeCheckstyle task
  • I added the SuppressWarningsFilter and SuppressWarningsHolder rules so that the @SuppressWarnings annotation can be used to silence Checkstyle checks. We shouldn't typically need this, but it seemed like a useful thing to configure. In contrast to the suppressions file, this approach makes the suppression obvious.
  • I changed the :configureIdeCheckstyle task to pull in rules from checkstyle_ide_fragment.xml and merged them into the generated config. The rules are as follows:
    • BooleanNegation - a task defined using DescendantToken that detects cases of ! being used negate a boolean expression. This ought to be in the main config, but at present we have a number of violations in the source
    • MissingJavadocMethod - requires that public methods are documented. I set minLineCount to 2 so that the rule doesn't trigger on simple methods.
    • MissingJavadocPackage - require Javadoc in package-info.java
    • MissingJavadocType - require types to be documented
    • JavadocMethod - require params and return type to be documeted

Now that Checkstyle can be made useful in an IDE, add extra rules so
that a contributor is given extra help when editing files, without
having the checkstyle task spam the console when running gradle.
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Jan 18, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should work towards enforcing == false. This is almost like tribal knowledge and we enforce it inconsistently. It's a very strait forward rule, and we could update instances of it pretty easily.

Also related to that, IntelliJ itself has a conflicting inspection. Basically, it highlights instances of == false and indicates those could be simplified. Would be nice if we could configure that inspection to be disabled for folks somehow.

@pugnascotia
Copy link
Contributor Author

Ack on updating the code proactively. It affects hundreds of files though, so we'll need to do that proactively.

I can see that the IntelliJ inspection is already disabled in .idea/inspectionProfiles/Project_Default.xml but I can't remember if IntelliJ picks that up or not when you import the project.

@pugnascotia pugnascotia merged commit 82fdec1 into elastic:master Jan 21, 2021
@pugnascotia pugnascotia deleted the extra-checkstyle-in-ide branch January 21, 2021 09:39
pugnascotia added a commit that referenced this pull request Jan 21, 2021
Now that Checkstyle can be made useful in an IDE, add extra rules only when
checking in the IDE so that a contributor is given extra help when editing
files, without having the checkstyle task spam the console when running gradle.

Apart from the `BooleanNegation` rule below, the new rules have the `warning`
severity level. The new Javadoc rules reflect the guidelines in
`CONTRIBUTING.md` that we've had for some time.

   * I upgraded Checkstyle, so now it interprets the config file in the same was
     as the IntelliJ plugin. That means that I could move the `LineLength` rule
     up a level and remove its special handling in the `:configureIdeCheckstyle`
     task
   * I added the `SuppressWarningsFilter` and `SuppressWarningsHolder` rules so
     that the `@SuppressWarnings` annotation can be used to silence Checkstyle
     checks. We shouldn't typically need this, but it seemed like a useful thing
     to configure. In contrast to the suppressions file, this approach makes the
     suppression obvious.
   * I changed the `:configureIdeCheckstyle` task to pull in rules from
     `checkstyle_ide_fragment.xml` and merged them into the generated config.
     The rules are as follows:
      * `BooleanNegation` - a task defined using `DescendantToken` that detects
        cases of `!` being used negate a boolean expression. This ought to be in
        the main config, but at present we have a number of violations in the
        source
      * `MissingJavadocMethod` - requires that public methods are documented. I
        set `minLineCount` to 2 so that the rule doesn't trigger on simple
        methods.
      * `MissingJavadocPackage` - require Javadoc in `package-info.java`
      * `MissingJavadocType` - require types to be documented
      * `JavadocMethod` - require params and return type to be documeted
@pugnascotia
Copy link
Contributor Author

Backported to 7.x in 770d9cd

@pugnascotia
Copy link
Contributor Author

I started working towards enforcing == false in #67817.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Tooling Developer tooliing and automation >enhancement Team:Delivery Meta label for Delivery team v7.12.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants