I18n Sniff: Only recognize the space as a padding character when accompanied by a width specifier. #777

Merged
merged 1 commit into from Jan 9, 2017

Projects

None yet

3 participants

@jrfnl
Contributor
jrfnl commented Jan 7, 2017

This PR implements the third option mentioned in #776 (comment)

... only recognize the optional padding character if there is also a width specifier in the placeholder. This could potentially mean that some valid placeholders would no longer be recognized as such.

Fixes #776

  • Includes additional unit test.
  • Includes detailed documentation of the regular expressions as they were getting quite hard to manage without.
@jrfnl jrfnl I18n Sniff: Only recognize the space as a padding character when acco…
…mpanied by a width specifier.

* Includes additional unit test.
* Includes detailed documentation of the regular expressions as they were getting quite hard to manage without.
8420c3c
*/
- const SPRINTF_PLACEHOLDER_REGEX = '/(?:(?<!%)(%(?:[0-9]+\$)?[+-]?(?:[ 0]|\'.)?-?[0-9]*(?:\.[0-9]+)?[bcdeEufFgGosxX]))/';
+ const SPRINTF_PLACEHOLDER_REGEX = '/(?:
@GaryJones
GaryJones Jan 8, 2017 edited Member

Is there any change to these regexes, beyond breaking it up into multiple lines? If so, could you highlight them please?

@jrfnl
jrfnl Jan 8, 2017 Contributor

Yes, there is (in both regexes).

only recognize the optional padding character if there is also a width specifier in the placeholder

The change is that the space as a padding character is now only recognized as such if accompanied by a width specifier. Other padding characters are still recognized as normal even when there is no width specifier.

In detail:
Line 34 - The space as a padding character has been removed.
Line 38 - 42 - this is the new part where the space is now only recognized as a padding character if there is also a width specifier
Line 37 + 42: the specs mention an optional padding character which can be added to the precision specifier, this was previously unaccounted for. Was (?:\.[0-9]+)?, changed to account for the padding option here.

@jrfnl
jrfnl Jan 8, 2017 Contributor

Oh and just to be clear - the first two changes in the regex is what allows the new unit test / test case by @ocean90 to pass correctly.

@jrfnl jrfnl added this to the 0.11.0 milestone Jan 8, 2017
@JDGrimes JDGrimes merged commit 0992dcb into develop Jan 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@JDGrimes JDGrimes deleted the feature/issue-776-regex branch Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment