PSR2/SwitchDeclarationSniff: use placeholders in error messages - #1447
PSR2/SwitchDeclarationSniff: use placeholders in error messages#1447ntdiary wants to merge 1 commit into
Conversation
| $phpcsFile->addError($error, $nextCase, 'WrongOpener', [strtoupper($type)]); | ||
| } else { | ||
| $phpcsFile->addError($error, $nextCase, 'WrongOpener' . $type); | ||
| $phpcsFile->addError($error, $nextCase, 'WrongOpener' . $type, [strtoupper($type)]); |
There was a problem hiding this comment.
I have two small questions:
- There are 8
strtoupper($type)in this file. Is it okay to add a$typeUcvar, or just keep it as-is? BTW, I've mostly seenxxxLcin the codebase, but notxxxUc. - This line isn't covered by tests, so I assume there might be some room for improvement here. e.g.
- Could add a test case for it.
- If no valid case exists, but this sniff is supposed to provide message, we might slightly adjust the above logic, since
scope_openermay not always exist. - Or if this is sth that should be handled as a parse error, maybe we don't event need this part.
I'm still learning this, so if this question is too basic, feel free to ignore it. I can just leave it as a note. :D
There was a problem hiding this comment.
@ntdiary Sorry for my slow reply.
- There are 8
strtoupper($type)in this file. Is it okay to add a$typeUcvar, or just keep it as-is?
This would absolutely be welcome and sounds like a good change which can be included in this PR.
BTW, I've mostly seen
xxxLcin the codebase, but notxxxUc.
This sniff uses UC for historic reasons. I see no reason to change it at this time though.
This line isn't covered by tests, so I assume there might be some room for improvement here. e.g.
- Could add a test case for it.
- If no valid case exists, but this sniff is supposed to provide message, we might slightly adjust the above logic, since
scope_openermay not always exist.- Or if this is sth that should be handled as a parse error, maybe we don't event need this part.
I wonder if that line is effectively dead code or if there is a test case we can throw at it.
Please feel free to investigate & get creative with test case and I'll happily look at a PR related to the issues you point out. I do believe this should be addressed in a separate PR though (different decision point).
Replace string concatenation with placeholders to follow the best practice. - `defaultNotLower` and `caseNotLower` error messages now expose 3 data values (previously 2). - `SpaceBeforeColonDEFAULT` and `SpaceBeforeColonCASE` error messages now expose 1 data value (previously 0). - `BodyOnNextLineDEFAULT` and `BodyOnNextLineCASE` error messages now expose 1 data value (previously 0). - `WrongOpenerdefault` and `WrongOpenercase` error messages now expose 1 data value (previously 0).
b3f22b4 to
b30abce
Compare
There was a problem hiding this comment.
@ntdiary Thank you for this PR. Looking good.
Please let me know if you'd like to address the duplicate strtoupper($type) function calls in this PR or if you want to move that to a separate PR. Both are valid choices and either is fine by me.
Edit: for the record - I've checked and this sniff is not extended, so these changes do not affect any other sniffs.
Description
Replace string concatenation with placeholders to follow the best practice.
Suggested changelog entry
Changed:
defaultNotLowerandcaseNotLowererror messages now expose 3 data values (previously 2).SpaceBeforeColonDEFAULTandSpaceBeforeColonCASEerror messages now expose 1 data value (previously 0).BodyOnNextLineDEFAULTandBodyOnNextLineCASEerror messages now expose 1 data value (previously 0).WrongOpenerdefaultandWrongOpenercaseerror messages now expose 1 data value (previously 0).Related issues/external references
related to #1240
Types of changes
PR checklist
Other than the line mentioned in the comment below, everything else is already covered by
PSR2/Tests/ControlStructures/SwitchDeclarationUnitTest.inc