Issue #16802: improve spelling and typo in CheckstyleAntTask
#16774
Conversation
Pankraz76
changed the title
Running Checkstyle on 1 files.Running Checkstyle on 1 files.
|
this might be a low hanging fruit, what do you think @nrmancuso? |
| new MessageLevelPair("Adding standalone file for audit", Project.MSG_VERBOSE), | ||
| new MessageLevelPair("To locate the files took \\d+ ms.", Project.MSG_VERBOSE), | ||
| new MessageLevelPair("Running Checkstyle on 1 files", Project.MSG_INFO), | ||
| new MessageLevelPair("Running Checkstyle on 1 files", Project.MSG_INFO), |
There was a problem hiding this comment.
Any reason to not use singular “file” here? This has always bothered me.
There was a problem hiding this comment.
Yes, I wanted to use "file(s)" but the test breaks due to regex matching. Using plural "files" would work better since we typically process multiple files in a codebase, though the test case specifically mentions singular.
Currently we have inconsistent usage of singular vs plural ("file" vs "files") when processing. This makes sense since we usually process multiple files, but the test expects singular.
I initially standardized on singular for consistency everywhere, but now I'm considering:
- Reverting to plural ("files") everywhere since it's more accurate
- Only applying singular ("file") in this specific test case
Which approach would you prefer?
Log messages should match
expected to match: Running Checkstyle on 1 file(s).
but was : Running Checkstyle on 1 file(s).
Looks like you want to use .isEqualTo() for an exact equality assertion.
| new MessageLevelPair("Using configuration file:.*", Project.MSG_VERBOSE), | ||
| new MessageLevelPair(auditStartedMessage, Project.MSG_DEBUG), | ||
| new MessageLevelPair(auditFinishedMessage, Project.MSG_DEBUG), | ||
| new MessageLevelPair("To process the files took \\d+ ms.", Project.MSG_VERBOSE), | ||
| new MessageLevelPair("File processing took \\d+ ms.", Project.MSG_VERBOSE), |
There was a problem hiding this comment.
It's more compact and less verbose (boilerplate), and more focused on what’s actually happening.
It also aligns with the existing pattern.
This sounds more professional to me.
There was a problem hiding this comment.
Any reason to not use singular “file” here? This has always bothered me.
now singular makes sense, as its very generic, simple and basic; making the plural overhead obsolet.
src/test/java/com/puppycrawl/tools/checkstyle/ant/CheckstyleAntTaskTest.java
Show resolved
Hide resolved
|
is this any good? @rdiachenko |
| @@ -277,6 +272,9 @@ public void execute() { | |||
| if (config == null) { | |||
| throw new BuildException("Must specify 'config'.", getLocation()); | |||
| } | |||
| final String version = CheckstyleAntTask.class.getPackage().getImplementationVersion(); | |||
| log("Checkstyle version: " + version, Project.MSG_VERBOSE); | |||
| log("Using config " + config, Project.MSG_VERBOSE); | |||
There was a problem hiding this comment.
This increases cohesion and reduces coupling, as the config file is now actually used here—we just verified the valid state for later use in logging. So, it makes sense to group these elements together, rather than where the usage was previously.
It also contributes to the narrative in the logging info, since the config file is somewhat related to the version; both are meta-information and therefore logically belong together.
| @@ -875,14 +875,14 @@ public final void testExecuteLogOutput() throws Exception { | |||
| final String auditFinishedMessage = bundle.getString(DefaultLogger.AUDIT_FINISHED_MESSAGE); | |||
|
|
|||
| final List<MessageLevelPair> expectedList = Arrays.asList( | |||
| new MessageLevelPair("checkstyle version .*", Project.MSG_VERBOSE), | |||
| new MessageLevelPair("Checkstyle version: .*", Project.MSG_VERBOSE), | |||
| new MessageLevelPair("Using config file:.*", Project.MSG_VERBOSE), | |||
There was a problem hiding this comment.
It also contributes to the narrative in the logging info, since the config file is somewhat related to the version; both are meta-information and therefore logically belong together.
| assertLoggedTime(loggedMessages, testingTime, "To locate the files"); | ||
| assertLoggedTime(loggedMessages, testingTime, "To process the files"); | ||
| assertLoggedTime(loggedMessages, testingTime, "File discovery took"); | ||
| assertLoggedTime(loggedMessages, testingTime, "File processing took"); |
There was a problem hiding this comment.
you can not take away without breaking
you can not spell clean without lean
Pankraz76
changed the title
Running Checkstyle on 1 files.CheckstyleAntTask
Pankraz76
changed the title
CheckstyleAntTaskspelling and fix whitespace typo in CheckstyleAntTask
Pankraz76
changed the title
spelling and fix whitespace typo in CheckstyleAntTaskspelling and fix whitespace typo in CheckstyleAntTask
Pankraz76
changed the title
spelling and fix whitespace typo in CheckstyleAntTaskspelling and fix whitespace typo in CheckstyleAntTask
|
@pankratz76 what issue are you trying to solve here? Please update description with a link to approved issue and make sure ci is green before we can proceed with review. |
Pankraz76
changed the title
spelling and fix whitespace typo in CheckstyleAntTaskspelling and typo in CheckstyleAntTask
created an dedicated issues for typo fix in #16802 |
|
space typo fix: #16806 |
Fix typo and simplify message in CheckstyleAntTask (issue checkstyle#16774)
Issue #16802: improve
spellingandtypoinCheckstyleAntTaskfix whitespace typo
Running Checkstyle on 1 files.Running Checkstyle on 1 files.I was wondering about this issue myself and found it accidentally while refactoring this class in #16772.