Fixing plural/singular issues in the po parser #11508
Conversation
| @@ -65,39 +65,39 @@ public function testParse() | |||
| $expected = [ | |||
| 'Plural Rule 1' => [ | |||
| '_context' => [ | |||
| '' => 'Plural Rule 1 (translated)', | |||
| ], | |||
There was a problem hiding this comment.
You shouldnt revert the trailing commas IMO, just too much diff changes then that are none.
There was a problem hiding this comment.
Seriously? All the other lines haven't had one I think and AFAIK best practice by CS is to not have it there?
There was a problem hiding this comment.
No best practice (what the sniffer should be enforcing) is to always have the multiline trailing commas, to keep diffs low exactly for the reasons shown in this PR: Too much unrelated noise around the few lines of actual change.
There was a problem hiding this comment.
They aren't used everywhere either here! https://github.com/cakephp/cakephp/blob/master/tests/TestCase/I18n/Parser/PoFileParserTest.php#L59-L102 So either remove them all or add them all!? However both ways will cause "noise".
There was a problem hiding this comment.
I guess in that case you can ignore my comment.
There was a problem hiding this comment.
We should agree on something and putt it into the coding standard? And then fix it everywhere in a separate PR. What do you think?
There was a problem hiding this comment.
Yeah, but can then be unrelated to this PR.
Codecov Report
@@ Coverage Diff @@
## master #11508 +/- ##
============================================
+ Coverage 93.37% 93.4% +0.02%
- Complexity 13021 13038 +17
============================================
Files 436 436
Lines 32755 32807 +52
============================================
+ Hits 30586 30642 +56
+ Misses 2169 2165 -4
Continue to review full report at Codecov.
|
| '_context' => [ | ||
| 'Another Context' => [ | ||
| 0 => '%d = 1 (translated)', | ||
| 1 => '%d = 0 or > 1 (translated)' | ||
| (int)0 => '%d = 1 (translated)', |
There was a problem hiding this comment.
I dont think you need the (int) cast here on the keys.
| @@ -22,6 +22,8 @@ | |||
| class Translator extends BaseTranslator | |||
| { | |||
|
|
|||
| const PLURAL_PREFIX = 'p:'; | |||
There was a problem hiding this comment.
Could we have one copy of the constant?
There was a problem hiding this comment.
Done, used the one here over the one in the parser.
| $message = $this->getMessage(static::PLURAL_PREFIX . $key); | ||
| if (!$message) { | ||
| $message = $this->getMessage($key); | ||
| } |
There was a problem hiding this comment.
We'll need this for backwards compatibilty and userland defined message loaders/parsers.
There was a problem hiding this comment.
What exactly do you mean? Do I need to change something else here?
There was a problem hiding this comment.
No. I was just commenting on it for future us.
This will fix the case described here: #10730 (comment)