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

GenericWhitespaceCheck: Handling of whitespace between generic and record header #14502

Closed
sktpy opened this issue Feb 18, 2024 · 3 comments · Fixed by #14529
Closed

GenericWhitespaceCheck: Handling of whitespace between generic and record header #14502

sktpy opened this issue Feb 18, 2024 · 3 comments · Fixed by #14529

Comments

@sktpy
Copy link
Contributor

sktpy commented Feb 18, 2024

From #14497 (comment) (second last paragraph)

(Check Documentation)

Goal of this Issue:

  • Conclude whether whitespace between generic and record header should be considered as a violation or not.
  • Based on the conclusion necessary changes can be done.

How it works Now:
Currently whitespace between generic and record header is not considered a violation.

Test.java

record Test<T> () {}
//            ^
//            +--- whitespace in question

config.xml

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">
<module name="Checker">
  <module name="TreeWalker">
    <module name="GenericWhitespace"/>
  </module>
</module>

Current CLI Execution:

java -jar checkstyle-10.13.0-all.jar -c config.xml Test.java
Starting audit...
Audit done.

Expected Working: To be decided
[ERROR] Test.java:1:14: '>' is followed by whitespace. [GenericWhitespace]

Findings that may be useful:

  • The syntax is kind of similar to Issue #14344
  • Horizontal Whitespace from google style, which includes the only places where occurrence of whitespace is valid does not includes this usage.
  • In oracle docs for record classes [see first point of Features of Record Classes], a whitespace is present between generic and record header in the example.
  • This check adheres to the "typical convention".

Typical Usages of Generic Records (Anecdotal):

  • Generic records in Checkstyle, default projects for regression ,and those I have encountered so far, do not contain whitespace between generic and record header.
    This can be verified by running the following two grep cmds:
    1. grep -P -r "record [a-zA-Z0-9\s$_]+\<.*\>\(.*\)" dir/ | tee >(wc -l)
    2. grep -P -r "record [a-zA-Z0-9\s$_]+\<.*\>\s+\(.*\)" dir/ | tee >(wc -l)
  • The pattern will match for the generic records, first one matching no-whitespace between generic and record header, while the second one matching atleast a single-whitespace between generic and record header.
  • Output for the cmd on Checkstyle repo: i. 32; ii: 0
  • Output for the cmd on regression repos: i. 60; ii. 0
    Side note: Arguably better way to verify that there are no such cases would be implement changes which detects space between generic and record header as a violation, and then check for differences in the report. (lmk if that is needed)
@sktpy
Copy link
Contributor Author

sktpy commented Feb 18, 2024

In my opinion, whitespace between generic and record header should be considered as a violation, only contradicting thing that I have encountered so far is that single example in the oracle docs.

@romani
Copy link
Member

romani commented Feb 18, 2024

yes, issue is approved

@sktpy
Copy link
Contributor Author

sktpy commented Feb 18, 2024

I am on it.

sktpy added a commit to sktpy/checkstyle that referenced this issue Feb 23, 2024
sktpy added a commit to sktpy/checkstyle that referenced this issue Feb 23, 2024
sktpy added a commit to sktpy/checkstyle that referenced this issue Feb 24, 2024
romani pushed a commit that referenced this issue Mar 15, 2024
@github-actions github-actions bot added this to the 10.14.2 milestone Mar 15, 2024
@romani romani added the bug label Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants