-
Notifications
You must be signed in to change notification settings - Fork 967
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
Added optional Culture parameter to DateTime.Humanize #286
Changes from 1 commit
530b2f5
adea66c
3dde23d
abbc23e
0788a38
ed4121c
1f8ad39
3bb7fec
464e7b4
70f8f72
c8bcd87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
using System; | ||
using System.Globalization; | ||
using System.Threading; | ||
using Humanizer.Configuration; | ||
using Humanizer.DateTimeHumanizeStrategy; | ||
using Humanizer.Localisation; | ||
|
@@ -8,26 +10,33 @@ namespace Humanizer.Tests | |
{ | ||
public class DateHumanize | ||
{ | ||
static void VerifyWithCurrentDate(string expectedString, TimeSpan deltaFromNow) | ||
private static void VerifyWithCurrentDate(string expectedString, TimeSpan deltaFromNow) | ||
{ | ||
var utcNow = DateTime.UtcNow; | ||
var localNow = DateTime.Now; | ||
CheckWithExplicitAndImplicitCulture(culture => | ||
{ | ||
var utcNow = DateTime.UtcNow; | ||
var localNow = DateTime.Now; | ||
|
||
// feels like the only way to avoid breaking tests because CPU ticks over is to inject the base date | ||
Assert.Equal(expectedString, utcNow.Add(deltaFromNow).Humanize(utcDate: true, dateToCompareAgainst: utcNow)); | ||
Assert.Equal(expectedString, localNow.Add(deltaFromNow).Humanize(utcDate: false, dateToCompareAgainst: localNow)); | ||
// feels like the only way to avoid breaking tests because CPU ticks over is to inject the base date | ||
Assert.Equal(expectedString, utcNow.Add(deltaFromNow).Humanize(true, utcNow, culture)); | ||
Assert.Equal(expectedString, localNow.Add(deltaFromNow).Humanize(false, localNow, culture)); | ||
}); | ||
} | ||
|
||
static void VerifyWithDateInjection(string expectedString, TimeSpan deltaFromNow) | ||
private static void VerifyWithDateInjection(string expectedString, TimeSpan deltaFromNow) | ||
{ | ||
var utcNow = new DateTime(2013, 6, 20, 9, 58, 22, DateTimeKind.Utc); | ||
var now = new DateTime(2013, 6, 20, 11, 58, 22, DateTimeKind.Local); | ||
CheckWithExplicitAndImplicitCulture(culture => | ||
{ | ||
var utcNow = new DateTime(2013, 6, 20, 9, 58, 22, DateTimeKind.Utc); | ||
var now = new DateTime(2013, 6, 20, 11, 58, 22, DateTimeKind.Local); | ||
|
||
Assert.Equal(expectedString, utcNow.Add(deltaFromNow).Humanize(utcDate: true, dateToCompareAgainst: utcNow)); | ||
Assert.Equal(expectedString, now.Add(deltaFromNow).Humanize(false, now)); | ||
Assert.Equal(expectedString, utcNow.Add(deltaFromNow).Humanize(true, utcNow, culture)); | ||
Assert.Equal(expectedString, now.Add(deltaFromNow).Humanize(false, now, culture)); | ||
}); | ||
} | ||
|
||
public static void Verify(string expectedString, int unit, TimeUnit timeUnit, Tense tense, double? precision = null) | ||
public static void Verify(string expectedString, int unit, TimeUnit timeUnit, Tense tense, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please format the method signature; i.e. precision to be moved to the end of the line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, sorry. I will fix it. But i just used included ReSharper config. May be it misses this rule and then it defaults to the rule defined on my PC's level? I'm not sure though. |
||
double? precision = null) | ||
{ | ||
if (precision.HasValue) | ||
Configurator.DateTimeHumanizeStrategy = new PrecisionDateTimeHumanizeStrategy(precision.Value); | ||
|
@@ -68,5 +77,14 @@ public static void Verify(string expectedString, int unit, TimeUnit timeUnit, Te | |
VerifyWithCurrentDate(expectedString, deltaFromNow); | ||
VerifyWithDateInjection(expectedString, deltaFromNow); | ||
} | ||
|
||
private static void CheckWithExplicitAndImplicitCulture(Action<CultureInfo> action) | ||
{ | ||
action(null); | ||
|
||
CultureInfo culture = Thread.CurrentThread.CurrentUICulture; | ||
using (new AmbientCulture(culture.TwoLetterISOLanguageName == "da" ? "tr" : "da")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it only between "da" and "tr"? I don't get this logic here!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idea here is that current thread's UI culture is changed to Danish (or to Turkey when current value is Danish). This way we ensure that subsequent Humanize calls with explicitly specified culture actually work. If we do not change current thread's UI culture before that, we can not be sure what culture has been actually used by Humanizer - one provided via param or one set in Thread.CurrentUiCulture. I hope it now makes more sense :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry. It doesn't. I am wondering why we should enforce this here at all. These tests were created to deal with ambient culture which is what happens in each localized tests with passing the desired culture to the base class. So I don't think we should change that behavior. What is also happening here is that we're running two sets of assertions: one for the ambient culture and one for the explicit one, which is going to slow down the test unnecessarily. IMO we should just cover this change separately using a couple of unit tests. You could put those in the test file in the root (that covers the English culture). I hope we're both talking about the same thing! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, let me try once more.
then we can not really know if Humanize() used "culture" parameter or CurrentUiCulture set by AmbientCulture call in test's constructor. So, to avoid this, i use new AmbientCulture region to change CurrentUiCulture to something different. Now, if we have a bug in DateTime.Humanize() and some branches of code do not use explicit "culture" parameter, we will end up with asserting failure because Humanize() used CurrentUiCulture which is set to a Culture other than we are testing at the moment. Regarding test speed.. i see your concern. But entire test suit still runs ~3sec, so is it really the issue? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implicit and explicit culture implementations are done in one place only and are shared across cultures. So you don't need to verify that for every single culture. If it works for two cultures it will be good for all. So I still think this doesn't belong to this class and we shouldn't disrupt/change the tests that were written for verifying the localisations. We are verifying something completely different here, and that's what I don't like. Please roll back the changes made to this class, and create a few separate explicit tests to verify the implicit vs explicit cultures. Thanks. |
||
action(culture); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,15 +14,15 @@ public DateHumanizeTests() : base("nl-NL") { } | |
[InlineData(-1, "gisteren")] | ||
public void DaysAgo(int days, string expected) | ||
{ | ||
Assert.Equal(expected, DateTime.UtcNow.AddDays(days).Humanize()); | ||
DateHumanize.Verify(expected, days, TimeUnit.Day, Tense.Past); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for fixing these. |
||
} | ||
|
||
[Theory] | ||
[InlineData(-2, "2 uur geleden")] | ||
[InlineData(-1, "één uur geleden")] | ||
public void HoursAgo(int hours, string expected) | ||
{ | ||
Assert.Equal(expected, DateTime.UtcNow.AddHours(hours).Humanize()); | ||
DateHumanize.Verify(expected, hours, TimeUnit.Hour, Tense.Past); | ||
} | ||
|
||
[Theory] | ||
|
@@ -39,23 +39,23 @@ public void MinutesAgo(int minutes, string expected) | |
[InlineData(-1, "één maand geleden")] | ||
public void MonthsAgo(int months, string expected) | ||
{ | ||
Assert.Equal(expected, DateTime.UtcNow.AddMonths(months).Humanize()); | ||
DateHumanize.Verify(expected, months, TimeUnit.Month, Tense.Past); | ||
} | ||
|
||
[Theory] | ||
[InlineData(-2, "2 seconden geleden")] | ||
[InlineData(-1, "één seconde geleden")] | ||
public void SecondsAgo(int seconds, string expected) | ||
{ | ||
Assert.Equal(expected, DateTime.UtcNow.AddSeconds(seconds).Humanize()); | ||
DateHumanize.Verify(expected, seconds, TimeUnit.Second, Tense.Past); | ||
} | ||
|
||
[Theory] | ||
[InlineData(-2, "2 jaar geleden")] | ||
[InlineData(-1, "één jaar geleden")] | ||
public void YearsAgo(int years, string expected) | ||
{ | ||
Assert.Equal(expected, DateTime.UtcNow.AddYears(years).Humanize()); | ||
DateHumanize.Verify(expected, years, TimeUnit.Year, Tense.Past); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,11 +23,20 @@ public LocaliserRegistry(TLocaliser defaultLocaliser) | |
} | ||
|
||
/// <summary> | ||
/// Gets the localiser for the current UI culture | ||
/// Gets the localiser for the current thread's UI culture | ||
/// </summary> | ||
public TLocaliser ResolveForUiCulture() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we even need this guy anymore? We could just replace the calls with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sure i plan to do that after i add Culture to other Humanize() methods. I just did not want to change anything unrelated to DateTime for now. |
||
{ | ||
var culture = CultureInfo.CurrentUICulture; | ||
return ResolveForCulture(); | ||
} | ||
|
||
/// <summary> | ||
/// Gets the localiser for the specified culture | ||
/// </summary> | ||
/// <param name="culture">The culture to retrieve localiser for. If not specified, current thread's UI culture is used.</param> | ||
public TLocaliser ResolveForCulture(CultureInfo culture = null) | ||
{ | ||
culture = culture ?? CultureInfo.CurrentUICulture; | ||
|
||
Lazy<TLocaliser> factory; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
using System; | ||
using System.Globalization; | ||
using Humanizer.Configuration; | ||
using Humanizer.Localisation; | ||
|
||
|
@@ -15,53 +16,56 @@ public class DefaultDateTimeHumanizeStrategy : IDateTimeHumanizeStrategy | |
/// </summary> | ||
/// <param name="input"></param> | ||
/// <param name="comparisonBase"></param> | ||
/// <param name="culture"></param> | ||
/// <returns></returns> | ||
public string Humanize(DateTime input, DateTime comparisonBase) | ||
public string Humanize(DateTime input, DateTime comparisonBase, CultureInfo culture) | ||
{ | ||
var tense = input > comparisonBase ? Tense.Future : Tense.Past; | ||
var ts = new TimeSpan(Math.Abs(comparisonBase.Ticks - input.Ticks)); | ||
|
||
var formatter = Configurator.GetFormatter(culture); | ||
|
||
if (ts.TotalMilliseconds < 500) | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Millisecond, tense, 0); | ||
return formatter.DateHumanize(TimeUnit.Millisecond, tense, 0, culture); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per the above suggestion, we wouldn't have to include culture in all these calls. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As i commented above: Yes, i also thought about this.But this means that for all those languages that do not have specific formatter and, therefore, use default instance of DefaultFormatter ... we have to create separate DefaultFormatter inside of FormatterRegistry's constructor. Do you think this makes sense? Essentialy this means that when adding support for a new language, we would have to add a line in FormatterRegistry's constructor. May be this is not that a big issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's a big issue, as for support a new language we anyway need to add tests and resources. With tests inplace you would quickly find the need to register the new language in the FormatterRegistry. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. Let's see if other guys agree as well. If they do, i will change this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont see any problems just pass current/supplied culture to default formatter. So 👍 for removing these parameters here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has been implemented as discussed above. Please review :). |
||
|
||
if (ts.TotalSeconds < 60) | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Second, tense, ts.Seconds); | ||
return formatter.DateHumanize(TimeUnit.Second, tense, ts.Seconds, culture); | ||
|
||
if (ts.TotalSeconds < 120) | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Minute, tense, 1); | ||
return formatter.DateHumanize(TimeUnit.Minute, tense, 1, culture); | ||
|
||
if (ts.TotalMinutes < 60) | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Minute, tense, ts.Minutes); | ||
return formatter.DateHumanize(TimeUnit.Minute, tense, ts.Minutes, culture); | ||
|
||
if (ts.TotalMinutes < 90) | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Hour, tense, 1); | ||
return formatter.DateHumanize(TimeUnit.Hour, tense, 1, culture); | ||
|
||
if (ts.TotalHours < 24) | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Hour, tense, ts.Hours); | ||
return formatter.DateHumanize(TimeUnit.Hour, tense, ts.Hours, culture); | ||
|
||
if (ts.TotalHours < 48) | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Day, tense, 1); | ||
return formatter.DateHumanize(TimeUnit.Day, tense, 1, culture); | ||
|
||
if (ts.TotalDays < 28) | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Day, tense, ts.Days); | ||
return formatter.DateHumanize(TimeUnit.Day, tense, ts.Days, culture); | ||
|
||
if (ts.TotalDays >= 28 && ts.TotalDays < 30) | ||
{ | ||
if (comparisonBase.Date.AddMonths(tense == Tense.Future ? 1 : -1) == input.Date) | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Month, tense, 1); | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Day, tense, ts.Days); | ||
return formatter.DateHumanize(TimeUnit.Month, tense, 1, culture); | ||
return formatter.DateHumanize(TimeUnit.Day, tense, ts.Days, culture); | ||
} | ||
|
||
if (ts.TotalDays < 345) | ||
{ | ||
int months = Convert.ToInt32(Math.Floor(ts.TotalDays / 29.5)); | ||
return Configurator.Formatter.DateHumanize(TimeUnit.Month, tense, months); | ||
return formatter.DateHumanize(TimeUnit.Month, tense, months, culture); | ||
} | ||
|
||
int years = Convert.ToInt32(Math.Floor(ts.TotalDays / 365)); | ||
if (years == 0) years = 1; | ||
|
||
return Configurator.Formatter.DateHumanize(TimeUnit.Year, tense, years); | ||
return formatter.DateHumanize(TimeUnit.Year, tense, years, culture); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking that maybe instead of adding culture to every single method we should add it as a property to
IFormatter
.Configurator
is creating the formatter with the culture. We might just stick it into a property or even field and use it from all the methods. That would minimize the impact on the API. Thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously you still need that on external facing API so the caller can pass in the desired culture in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i also thought about this.But this means that for all those languages that do not have specific formatter and, therefore, use default instance of DefaultFormatter ... we have to create separate DefaultFormatter inside of FormatterRegistry's constructor. Do you think this makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha! Cool.