Correct pluralization of the word space/spaces depending on the spacing value #3881
Conversation
There was a problem hiding this comment.
Thank you @DannyvdSluijs for creating this PR!
I've done a detailed review and tested the changes on the command line too.
Please see my inline review notes and let me know if you have any questions.
There was a problem hiding this comment.
This sniff has three distinct error messages, which each use the "spaces" phrasing.
The last one (close brace) has now been adjusted, but the other two messages still have the same issue.
I also noted that there are no tests covering this change as indent 1 isn't used in the test file. While this wouldn't make a difference for the automated tests (as those don't test the actual error message), not having those tests there means that the change cannot easily be tested manually either and that code coverage goes down.
There was a problem hiding this comment.
@jrfnl thanks for the review and detailed messages. You mentioned the other two cases not being updated. This is a good spot and also shows that the words being used in the messages can be all over the place. Based on your other PR I searched the code base for spaces being mentioned in the error message. Searching for %s spaces in the codebase shows a total of 42 occurrences. This PR was drafted by searching for space(s) and fixing those.
What do you thing would be the desired approach here? Fix all thespaces occurrences using this same PR or target those for another PR (which I'm happy to create as well)
With regards to the coverage going done due the missing test that is actually somewhat of a valid point. I can look into adding these where possible. My only concern would be the amount of cases where I needed to tweak some of the config of class property values to get to the correct scenario to return the specific error message. Any tips you can share on how to approach that?
Also looking ahead at your other comment I guess answering this would help on that instance as well.
Happy to tweak the PR but some nudge into the correct direction would be helpful.
There was a problem hiding this comment.
What do you think would be the desired approach here?
In my opinion, I'd keep this PR for the sniffs which are already being handled in it and would open separate PR(s) for changes to other sniffs. That way, this PR won't get blocked for merge by the new set of changes..
As a general rule of thumb, I normally try to keep PRs small and self-contained. That way, each PR can be reviewed and merged on their own merit. This prevents an incomplete or incorrect change to one file blocking the merge of the other changes, which may be perfectly ready to go.
It also makes it easier to review PRs, as a reviewer only needs to focus on one thing at a time and there is no risk of a PR getting blocked because it contains multiple different decision points (5 decisions: yes, 1: no, PR can't get merged).
Does that help ?
My only concern would be the amount of cases where I needed to tweak some of the config of class property values to get to the correct scenario to return the specific error message. Any tips you can share on how to approach that?
Well, to test the changes in the PR, I actually created some tests (locally) for the existing changes which weren't being hit by the tests. Would it help you if I would push those tests to this branch so you can see the tests I added (and how to change the sniff properties from within a test file) ?
There was a problem hiding this comment.
Okay, valid points indeed. Most just wanted to make sure we are aligned here.
So I'll make sure that for any file already touched in this PR the issues with the words spaces is also addressed.
Would it help you if I would push those tests to this branch so you can see the tests I added (and how to change the sniff properties from within a test file) ?
Yes if you can please or if pushing is too much of a hassle feel free to drop some diff content here and I can take it from there as well.
There was a problem hiding this comment.
No hassle, I check remote branches out locally anyway to test them. The tests should now show in your branch.
There was a problem hiding this comment.
Shoot! I just noticed the tests are failing now as I tested your changes in isolation (excluded the other error messages from the sniff). Let me fix that up. (I will force push the changes, so give me a moment before you pull the changes in)
There was a problem hiding this comment.
I've corrected all the plural cases on spaces in the files touched in this PR.
Looking at the tests and your additions I was mostly overwhelmed by the structure. How would you even check if specific cases are covered?
There was a problem hiding this comment.
Detailed notes:
- The change for the
CloseBraceNotAlignederror code works, but has no tests covering the change (no test where the error message would result in "expected 1 space"). - The other changes are all okay and covered by tests.
- The
KeyNotAlignederror code has similar message phrasing, but the logic for building that error message has not been updated (yet). Note: there are tests already covering this, so it would only need the code change.
| 1 => 'one', | ||
| ]; | ||
|
|
||
| // phpcs:set Generic.Arrays.ArrayIndent indent 4 |
There was a problem hiding this comment.
FYI: This second one is a "reset" where I set the property value back to the original value to prevent future tests being added running into an unexpected property value.
|
@DannyvdSluijs Could you please re-pull this PR (with a reference to this one) in the https://github.com/PHPCSStandards/PHP_CodeSniffer repo ? I'd like to try and include it in the 3.8.0 release. |
|
I’ll try and see if I can manage tonight. If not it should be somewhere this week. Thanks for the proactive stand and carrying through with PHPCS despite the hard times. If you think I can be of help in any other way feel free to tag me in an issue or PR. And I’ll see what I can do to help. |
|
This has been reopened in the continuation repository. See PHPCSStandards/PHP_CodeSniffer#128 |
This PR fixes more occurences of the word
space(s)to be eitherspaceorspacesdepending on the spacing value.Description
This PR has been created after some brief discussion with @jrfnl in #3647 and addressing the Sniff I've pointed out in #3647 (review)
Suggested changelog entry
Correct pluralization of the word space/spaces depending on the spacing value
Related issues/external references
Fixes #
Types of changes
PR checklist
package.xmlfile.