Add a check for the leading and trailing spaces #2504
Conversation
There was a problem hiding this comment.
Hi @amieiro, thanks for this PR.
I haven't tested it yet, but based on a code review alone, this is looking good.
I wonder if the tests can be made a little more varied ? I.e. not just use leading/trailing spaces, but also have some leading/trailing tabs, new lines and combinations of spaces, tabs, new lines.
What do you think ?
|
@amieiro Just checking in as you never responded to the question I posed in my initial review.... Do you still intend to finish off this PR ? |
|
Sorry, I don't have the time to finish it at this time. |
| array( $param_info['clean'] ) | ||
| ); | ||
| } | ||
|
|
| array( $param_info['clean'] ) | ||
| ); | ||
| } | ||
|
|
| array( $param_info['clean'] ) | ||
| ); | ||
| } | ||
|
|
| array( $param_info['clean'] ) | ||
| ); | ||
| } | ||
|
|
| array( $param_info['clean'] ) | ||
| ); | ||
| } | ||
|
|
| $pattern_trailing_spaces = '/[\x20]+$/u'; | ||
| $pattern_leading_tabs = '/^\x09+/u'; | ||
| $pattern_trailing_tabs = '/\x09+$/u'; | ||
| $pattern_leading_vtabs = '/^\x0B+/u'; |
There was a problem hiding this comment.
| $pattern_leading_vtabs = '/^\x0B+/u'; | |
| $pattern_leading_vtabs = '/^\x0B+/u'; |
| $pattern_leading_spaces = '/^[\x20]+/u'; | ||
| $pattern_trailing_spaces = '/[\x20]+$/u'; | ||
| $pattern_leading_tabs = '/^\x09+/u'; | ||
| $pattern_trailing_tabs = '/\x09+$/u'; |
There was a problem hiding this comment.
| $pattern_trailing_tabs = '/\x09+$/u'; | |
| $pattern_trailing_tabs = '/\x09+$/u'; |
| // Define regex patterns. | ||
| $pattern_leading_spaces = '/^[\x20]+/u'; | ||
| $pattern_trailing_spaces = '/[\x20]+$/u'; | ||
| $pattern_leading_tabs = '/^\x09+/u'; |
There was a problem hiding this comment.
| $pattern_leading_tabs = '/^\x09+/u'; | |
| $pattern_leading_tabs = '/^\x09+/u'; |
|
|
||
| // Define regex patterns. | ||
| $pattern_leading_spaces = '/^[\x20]+/u'; | ||
| $pattern_trailing_spaces = '/[\x20]+$/u'; |
There was a problem hiding this comment.
| $pattern_trailing_spaces = '/[\x20]+$/u'; | |
| $pattern_trailing_spaces. = '/[\x20]+$/u'; |
| $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param_info['start'], ( $param_info['end'] + 1 ), true ); | ||
|
|
||
| // Define regex patterns. | ||
| $pattern_leading_spaces = '/^[\x20]+/u'; |
There was a problem hiding this comment.
| $pattern_leading_spaces = '/^[\x20]+/u'; | |
| $pattern_leading_spaces = '/^[\x20]+/u'; |
|
@amieiro Seeing as you've been working on this again - could you please explain why you have set things up to have separate errors for spaces vs tabs vs vtabs vs newlines ? My previous question was only about having tests in place with these and the sniff handling all types of whitespace, not about splitting that up into multiple errors. I cannot think of any situation where one type of (leading/trailing) whitespace would be acceptable, but another would not be, so having different error codes doesn't make sense to me. Is there a reason for this distinction based on your experiences in the Polyglots team ? I'd be interested to hear your reasoning for this. |
|
@jrfnl Sorry, this PR is not yet ready for review and I forgot to convert it to draft. As we return to contribute with WordPress.org, I resumed work on this PR. The horizontal tabulations are not often seen at the beginning or end of sentences, but they are seen in the middle, so I wanted to prevent its use, since I don't think it is correct to use it. You can see a use case inside a string in the core here. The same applies to the new lines. You can see an example in the core here. The most common problem is the U+0020 space problem (in fact it is the only one I usually see), so the others are really edge situations. I can continue this PR in two different approaches:
Which approach do you prefer? |
Okay, thanks for letting me know. I've converted it to draft now and won't do an in-detail review until it has been marked as "Ready to review".
I think that this PR should probably focus on the original problem it was trying to solve - leading and trailing whitespace. Expanding the scope to mid-string whitespace characters feels like scope-creep and may have different implications (i.e., would involve a different decision point, potentially delaying the merging of the code related to the original problem). I suggest opening a follow-up issue for that to discuss if this needs flagging and if so, what would need flagging. (I mean, looking at the examples, these seem like perfectly valid use-cases for mid string vertical whitespace, especially if you also consider RTL and BTT languages).
For leading and trailing whitespace, IMO the new errors should flag all whitespace characters, but without differentiating in the error message or code.
Does that help ? |
As I described in #2501, at translate.w.org, sometimes we get translations that start or end with one or more empty spaces, and we want to avoid this.
This PR adds a new check to detect if a translatable string has leading or trailing spaces.
Fixes #2501.