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

IdentifierName rejects m-failsafe-p default inclusion patterns #4832

Open
commonquail opened this issue Feb 21, 2025 · 2 comments · May be fixed by #4842
Open

IdentifierName rejects m-failsafe-p default inclusion patterns #4832

commonquail opened this issue Feb 21, 2025 · 2 comments · May be fixed by #4842

Comments

@commonquail
Copy link

commonquail commented Feb 21, 2025

By default, the Maven Failsafe plugin relies on a set of file name patterns to determine which test classes to include. This is the <includes> option, and its default values are

  • **/IT*.java
  • **/*IT.java
  • **/*ITCase.java

In v2.36.0, the IdentifierName check (which I'm unreasonably excited about!) accurately detects those names as being in violation.

Unfortunately, for a user wishing to take advantage of Failsafe, working around this is a bit cumbersome; it seems they can...

  • ... not run Error Prone on test code at all, but I would like to.
  • ... change <includes> to any compliant non-default value, but that introduces non-standard behaviour for dubious reasons.
  • ... use -XepOpt:IdentifierName:AllowInitialismsInTypeName=true, but actually, that allows a name like XMLHTTPRequest that is specifically called out as in violation, and incidentally, is exactly the sort of situations I see people struggle with the most.
  • ... suppress each violation, which I personally consider the correct response but a noisy and tiresome one.

Every option is possible but all of them carry friction.

The behaviour I would most like to see is that IdentifierName automagically did not flag these cases so the defaults Just Work™. But Error Prone cannot know which arbitrary patterns I might have set in my own <includes>, so any support for this is bound to be either inflexible or complex. Correspondingly, I would suggest that IdentifierName should at most allow Failsafe's default <includes> values

A more flexible approach is a new option, -XepOpt:IdentifierName:MyBikeshed=*IT,*MXBean, which passes a set of patterns to IdentifierName to allow.

See also #4776.

$ ./mvnw clean test-compile -DepFlags='-XepDisableAllChecks -Xep:IdentifierName:ERROR'
[INFO] Scanning for projects...
[INFO]
...
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /.../src/test/java/example/ITFoo.java:[1,8] [IdentifierName] Classes should be named in UpperCamelCase, with acronyms treated as words (https://google.github.io/styleguide/javaguide.html#s5.3-camel-case); did you mean 'ItFoo'?
    (see https://google.github.io/styleguide/javaguide.html#s5.2-specific-identifier-names)
[ERROR] /.../src/test/java/example/FooIT.java:[3,8] [IdentifierName] Classes should be named in UpperCamelCase, with acronyms treated as words (https://google.github.io/styleguide/javaguide.html#s5.3-camel-case); did you mean 'FooIt'?
    (see https://google.github.io/styleguide/javaguide.html#s5.2-specific-identifier-names)
[ERROR] /.../src/test/java/example/FooITCase.java:[3,8] [IdentifierName] Classes should be named in UpperCamelCase, with acronyms treated as words (https://google.github.io/styleguide/javaguide.html#s5.3-camel-case); did you mean 'FooItCase'?
    (see https://google.github.io/styleguide/javaguide.html#s5.2-specific-identifier-names)
@graememorgan
Copy link
Member

which I'm unreasonably excited about!

I'm glad someone is unreasonably excited about style-violating identifier names. :)

As you can probably imagine, we're not super motivated about adding carve-outs for the style guide that don't matter to Google internally. I quite like the idea of a regular expression flag to exempt certain names, though, if that would be useful to external users. I'd be happy to accept a PR.

commonquail added a commit to commonquail/error-prone that referenced this issue Feb 25, 2025
Occasionally, type names need to follow conventions that violate the
styleguide without being able to rely on inheritance or annotations for
exemption. For example, JMX MBean and MXBean definitions will routinely
violate the styleguide, and outside the JDK, `maven-failsafe-plugin`
3.5.2's default filter is a set of 3 violating patterns. Suppressing
these violations quickly becomes tedious.

Rather than Whac-A-Mole'ing tolerated violations, define a new option
whose value is a regex such that, if a possibly-violating type name
matches that regex, the type name is allowed anyway.

References: google#4776
References: google#4832
@commonquail
Copy link
Author

As you can probably imagine, we're not super motivated about adding carve-outs for the style guide that don't matter to Google internally.

No doubt; and it can be pretty frustrating to be on the user side of fixed exemptions that don't include one's pet violation, too. Fortunately it was a lot easier to expose a regex option than I had guessed it would be; I have a preliminary suggestion in #4842.

commonquail added a commit to commonquail/error-prone that referenced this issue Feb 25, 2025
Occasionally, type names need to follow conventions that violate the
styleguide without being able to rely on inheritance or annotations for
exemption. For example, JMX MBean and MXBean definitions will routinely
violate the styleguide, and outside the JDK, `maven-failsafe-plugin`
3.5.2's default filter is a set of 3 violating patterns. Suppressing
these violations quickly becomes tedious.

Rather than Whac-A-Mole'ing tolerated violations, define a new option
whose value is a regex such that, if a possibly-violating type name
matches that regex, the type name is allowed anyway.

References: google#4776
References: google#4832
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 a pull request may close this issue.

2 participants