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

Fuzzy month years logic for time span humanize extensions #632

Conversation

MaStr11
Copy link
Contributor

@MaStr11 MaStr11 commented May 3, 2017

Fixes #583
This change now allows calls to TimeSpanHumanizeExtensions.Humanize with the parameter maxUnit and minUnit set to TimeUnit.Month or maxUnit: TimeUnit.Year. The duration in month and years is approximated based on 365.2425 days a year (length of the gregorian calendar). Therefore the month are alternating between 30 and 31 days and the years are alternating between 365 and 366 days.

The existing default value for maxUnit=TimeUnit.Week is kept because weeks are precise while month and year are not.

Most of the changes are related to the translation of the month and year identifiers. All text were translated with google translator except:

  • German I'm a native German.
  • Russian Reviewed by a native speaker.
  • fi-FI Tests and translations for timespan were already missing.
  • el Tests and translations for timespan were already missing.

I marked the test related to translations with the [Trait("Translation", "Google")] attribute to make clear which translations are of poor quality.

Checklist for the Translation Reviews:

Copy link
Contributor

@khellang khellang left a comment

Choose a reason for hiding this comment

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

Norwegian looks good. I left some comments on the Danish and Swedish translations. Hopefully @thecodejunkie and @madstt can verify my corrections.

<value>en måned</value>
</data>
<data name="TimeSpanHumanize_SingleYear" xml:space="preserve">
<value>en år</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

et år // @madstt?

Copy link

Choose a reason for hiding this comment

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

that is correct, it is called 'et år' in Danish, not 'en år'...

<value>{0} år</value>
</data>
<data name="TimeSpanHumanize_SingleMonth" xml:space="preserve">
<value>1 månad</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

en månad // @thecodejunkie?

<value>1 månad</value>
</data>
<data name="TimeSpanHumanize_SingleYear" xml:space="preserve">
<value>1 år</value>
Copy link
Contributor

@khellang khellang May 3, 2017

Choose a reason for hiding this comment

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

ett år // @thecodejunkie?

Copy link

@M-Zuber M-Zuber left a comment

Choose a reason for hiding this comment

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

Commented with fixes for the Hebrew translations (source: I speak it fluently)

<value>{0} שנים</value>
</data>
<data name="TimeSpanHumanize_MultipleYears_Dual" xml:space="preserve">
<value>שנים</value>
Copy link

Choose a reason for hiding this comment

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

2 years is either: 2 שנים or שנתיים

<value>{0} חודשים</value>
</data>
<data name="TimeSpanHumanize_MultipleMonths_Dual" xml:space="preserve">
<value>חודשים</value>
Copy link

Choose a reason for hiding this comment

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

2 months is either 2 חודשים or חודשיים


/// <summary>
/// Turns a TimeSpan into a human readable form. E.g. 1 day.
/// </summary>
/// <param name="timeSpan"></param>
/// <param name="precision">The maximum number of time units to return. Defaulted is 1 which means the largest unit is returned</param>
/// <param name="culture">Culture to use. If null, current thread's UI culture is used.</param>
/// <param name="maxUnit">The maximum unit of time to output.</param>
/// <param name="maxUnit">The maximum unit of time to output. The default value is <see cref="TimeUnit.Week"/>. The time units <see cref="TimeUnit.Month"/> and <see cref="TimeUnit.Year"/> will give approximations for time spans bigger 30 days by calculating with 365.2425 days a year and 30.4369 days a month.</param>
Copy link

Choose a reason for hiding this comment

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

for time spans bigger 30 days -> for time spans bigger than 30 days

<value>חודש</value>
</data>
<data name="TimeSpanHumanize_SingleYear" xml:space="preserve">
<value>שנה אחת</value>
Copy link

Choose a reason for hiding this comment

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

Can be shortened to just שנה

@M-Zuber
Copy link

M-Zuber commented May 4, 2017

Out of curiosity, as I can not pull down the code and test it myself,
what is displayed if the value is smaller than the TimeUnit passed?

@clairernovotny
Copy link
Member

Thanks @M-Zuber @khellang @thecodejunkie @madstt for jumping in! I'm trying to get a 2.2 release out by Friday afternoon, and I'd love to get this included if the language updates can be incorporated and validated by then :)

@clairernovotny clairernovotny added this to the v2.2 milestone May 4, 2017
@jeremymeng
Copy link

Chinese translations (all three) look good.

@herecydev
Copy link

Can confirm the Russian is all fine from a native speaker.

@MaStr11
Copy link
Contributor Author

MaStr11 commented May 4, 2017

@M-Zuber What is displayed if the value is smaller than the TimeUnit passed?
You can have a look at the test cases
TimeSpanWithMinTimeUnit and TimeSpanWithMaxTimeUnit. If minUnit==TimeUnit.Year and timeSpan<=365 than no time is returned. If maxUnit==TimeUnit.Month and timeSpan==366 than 12 months are returned.

…ked reviewed language with "Native speaker". Marked 'sv' as reviewed even confirmation is pending.
@thecodejunkie
Copy link

Swedish looks OK 👍

{

[Theory]
[Trait("Translation", "Google")]
Copy link
Contributor

Choose a reason for hiding this comment

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

These are also verified now 😄

@clairernovotny
Copy link
Member

@MaStr11 is this basically ready with the stated manual/machine translations?

@clairernovotny clairernovotny mentioned this pull request May 4, 2017
@NinoFloris
Copy link

Dutch (nl) looks totally fine

@ctyar
Copy link

ctyar commented May 4, 2017

Farsi translation looks good 👍

@MaStr11
Copy link
Contributor Author

MaStr11 commented May 4, 2017

@onovotny The current state of the Translations is:

Missing (timespan related translations are completely absent):

  • fi-FI
  • el

Reviewed by native speakers:

  • da (corrections pushed in commit 5a38789)
  • de
  • du
  • fa
  • he (corrections pushed in commit 5a38789)
  • nl
  • nb
  • ru
  • sv (corrections pushed in commit 5a38789)
  • zh

Others:

Translated by Google translate. I'm quite confident with the slavic anguages because I can read those and have a minor understanding of basic rules but africans, arabic, chinese korean japanese and the like are just copy&paste.

@clairernovotny clairernovotny changed the base branch from dev to pr632 May 4, 2017 14:31
@clairernovotny clairernovotny merged commit 0ab0d95 into Humanizr:pr632 May 4, 2017
@clairernovotny
Copy link
Member

merging into branch pr632 for others to easily fork/PR into it

clairernovotny pushed a commit that referenced this pull request May 4, 2017
#632 Continued: Fuzzy month years logic for time span humanize extensions

[Theory]
[Trait("Translation", "Google")]
[InlineData(366, "1 ani")]
Copy link

Choose a reason for hiding this comment

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

It should be "1 an"

Copy link
Member

Choose a reason for hiding this comment

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

can you please submit a PR? no PR is too small :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes are part of #636

@MaStr11 MaStr11 deleted the FuzzyMonthYearsLogicForTimeSpanHumanizeExtensions branch May 5, 2017 19:26
clairernovotny pushed a commit that referenced this pull request Nov 3, 2017
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.