Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add russian. Add prefix at format difference. #41

Merged
merged 7 commits into from
Nov 3, 2015

Conversation

sam002
Copy link
Contributor

@sam002 sam002 commented Oct 30, 2015

No description provided.

@norberttech
Copy link
Member

@sam002 thanks for contributing! Could you please take a look at #23? As far as I know @Forst was working on that, maybe he can help you here or at least check if what u did is valid?


return implode(', ', $diff).' '.$this->translator->trans($suffix, array(), 'difference', $locale);
return $prefixString . implode(', ', $diff) . $suffixString;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance you could write some a tests for PreciseFormatter? Now when we have also prefixes and suffixes it might be hard to understand this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In two days. It is generally safe, the error will be at address diff verification.

@sam002
Copy link
Contributor Author

sam002 commented Oct 31, 2015

@norzechowicz, all done. @juanramirez did a tests for formatting.

@norberttech
Copy link
Member

@sam002 @juanramirez thanks. Seems it worsk, but there is one thing I dont like in this code. Is the fact that in order to choose which version of "from now" we should use we need to translate "from_now" and if its not translated then we are going to "from_now_prefix". It looks a bit fragile to me.
I was looking at this code and I'm thinkg about making it a bit more like @Forst suggested in #23

Option 1

The idea is to basicaly create one translation string each time. So for example for past time it would be something like:

"compound.ago_prefix" . $value . "compound.ago_suffix"
"compound.from_now_prefix" . $value . "compound.from_now_suffix"

And for languages that dont use prefix we could simply have translations like:

compound:
  ago_prefix: ""
  ago_suffix: "ago"
  from_now_prefix: ""
  from_now_suffix: "from now"

Option 2

Or we can even make it more flexible by making past and future.

Example for suffixes

unit:
  second: "{1} %count% second|[2,Inf] %count% seconds"
  minute: "{1} %count% minute|[2,Inf] %count% minutes"
  hour: "{1} %count% hour|[2,Inf] %count% hours"
  day: "{1} %count% day|[2,Inf] %count% days"
  week: "{1} %count% week|[2,Inf] %count% weeks"
  month: "{1} %count% month|[2,Inf] %count% months"
  year: "{1} %count% year|[2,Inf] %count% years"
past: "%values% ago"
future: "%value% from now"
$this->transChoice("past", ["%value%" => "2 hours, 3 seconds"]);
$this->transChoice("future", ["%value%" => "2 hours, 3 seconds"]);

// "2 hours, 3 seconds ago"
// "2 hours, 3 seconds from now"

Example for prefixes

unit:
  second: "{1} %count% second|[2,Inf] %count% seconds"
  minute: "{1} %count% minute|[2,Inf] %count% minutes"
  hour: "{1} %count% hour|[2,Inf] %count% hours"
  day: "{1} %count% day|[2,Inf] %count% days"
  week: "{1} %count% week|[2,Inf] %count% weeks"
  month: "{1} %count% month|[2,Inf] %count% months"
  year: "{1} %count% year|[2,Inf] %count% years"
past: "xxx %values%"
future: "in %value%"
$this->transChoice("past", ["%value%" => "2 hours, 3 seconds"]);
$this->transChoice("future", ["%value%" => "2 hours, 3 seconds"]);

// "xxx 2 hours, 3 seconds"
// "za 2 hours, 3 seconds"

And we can go even one step further and use Oxford mechanism to create value. This will give us something like 2 hours and 3 seconds ago instead of 2 hours, 3 seconds ago but this can be done in next PR.

@sam002
Copy link
Contributor Author

sam002 commented Nov 2, 2015

Yes, option 2 is the pretty way! I has implemented the past/future formatter and merged repos.
Plz, check: spec/Coduo/PHPHumanizer/DateTime/PreciseFormatterSpec.php Do now this checks?

$translationKey = $difference->isPast() ? 'past' : 'future';

return $this->translator->trans(
'compound.' . $translationKey,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for $translationKey, 'compound.' . $difference->isPast() ? 'past' : 'future' its enoguh

@norberttech
Copy link
Member

@sam002 now it looks much better! Thanks!

norberttech pushed a commit that referenced this pull request Nov 3, 2015
Add russian. Add prefix at format difference.
@norberttech norberttech merged commit a4d68a9 into coduo:master Nov 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants