-
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
Issue #101: Draft for precision-based calculator #114
Changes from all commits
03d130a
fb7ab0a
c4c6a9d
4cfe4b1
de165c5
ced243f
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 |
---|---|---|
@@ -0,0 +1,43 @@ | ||
using System; | ||
using Humanizer.Configuration; | ||
using Humanizer.DistanceOfTimeCalculators; | ||
using Xunit; | ||
|
||
namespace Humanizer.Tests | ||
{ | ||
public abstract class DateHumanizeTestsBase : IDisposable | ||
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. It's good this is extracted out as we need this in #123. |
||
{ | ||
private readonly IDistanceOfTimeInWords previousCalculator; | ||
|
||
protected DateHumanizeTestsBase() | ||
{ | ||
previousCalculator = Configurator.DistanceOfTimeInWords; | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
Configurator.DistanceOfTimeInWords = previousCalculator; | ||
} | ||
|
||
protected static void Verify(string expectedString, TimeSpan deltaFromNow) | ||
{ | ||
VerifyWithCurrentDate(expectedString, deltaFromNow); | ||
VerifyWithDateInjection(expectedString, deltaFromNow); | ||
} | ||
|
||
private static void VerifyWithCurrentDate(string expectedString, TimeSpan deltaFromNow) | ||
{ | ||
Assert.Equal(expectedString, DateTime.UtcNow.Add(deltaFromNow).Humanize()); | ||
Assert.Equal(expectedString, DateTime.Now.Add(deltaFromNow).Humanize(false)); | ||
} | ||
|
||
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); | ||
|
||
Assert.Equal(expectedString, utcNow.Add(deltaFromNow).Humanize(dateToCompareAgainst: utcNow)); | ||
Assert.Equal(expectedString, now.Add(deltaFromNow).Humanize(false, now)); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
using System; | ||
using Humanizer.DistanceOfTimeCalculators; | ||
using Humanizer.Localisation; | ||
using Xunit; | ||
|
||
namespace Humanizer.Tests.DistanceOfTimeCalculators | ||
{ | ||
public class DefaultPrecisionBoundaries | ||
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. And I just had an aha moment. I never thought about the logic behind DateTime Humanize breakpoints; but looking at your breakpoints here made me realize it is calculated on a .75 precision! So I think having two separate strategies for DateTime calculation feels redundant as they're basically the same. The only difference is that in the default one the breakpoints are hardcoded while in the precision based one they're calculated. This makes me wonder: do we even need the default one? I think we should just replace the default implementation with the precision based calculator, and we don't need to create the interface or make this configurable beyond providing the desired precision. 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 that the default one is .75 for all values. We can validate this by tests, just swap the calculator & run same tests ;) The point, I was thinking to introduce the change in a non-breaking way. So deliberately anyone wants to leverage the new precision-based calculator can do this, if not he's fine & nothing changed for him. |
||
{ | ||
readonly PrecisionBasedDistanceOfTime _calculator = new PrecisionBasedDistanceOfTime(); | ||
|
||
private readonly string justNow = GetJustNow(); | ||
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. Not sure why this had to get its separate method while other resources were fetched inline?! Maybe this should be inlined too. |
||
private readonly string secondFromNow = GetResource(TimeUnit.Second, 1); | ||
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. In fact I am not sure why these are created here and not in the test methods they're used as each field is used only once. |
||
private readonly string minuteFromNow = GetResource(TimeUnit.Minute, 1); | ||
private readonly string hourFromNow = GetResource(TimeUnit.Hour, 1); | ||
private readonly string dayFromNow = GetResource(TimeUnit.Day, 1); | ||
private readonly string monthFromNow = GetResource(TimeUnit.Month, 1); | ||
private readonly string yearFromNow = GetResource(TimeUnit.Year, 1); | ||
|
||
[Fact] | ||
public void MillisecondsBorder() | ||
{ | ||
const int millisecondsBoder = 750; | ||
VerifyBorder(TimeSpan.FromMilliseconds, millisecondsBoder, justNow, secondFromNow); | ||
} | ||
|
||
[Fact] | ||
public void FourtyFiveSecondsBorder() | ||
{ | ||
const int secondsBorder = 45; | ||
VerifyBorder(TimeSpan.FromSeconds, secondsBorder, | ||
GetResource(TimeUnit.Second, secondsBorder - 1), | ||
minuteFromNow); | ||
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. It would be easier to read the test in my opinion if we got the |
||
} | ||
|
||
[Fact] | ||
public void FourtyFiveMinutesBorder() | ||
{ | ||
const int minutesBorder = 45; | ||
VerifyBorder(TimeSpan.FromMinutes, minutesBorder, | ||
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. Once you break the argument list into lines, it's easier on eye if each argument appeared on a separate line. So I think this should ultimately be reformatted to:
|
||
GetResource(TimeUnit.Minute, minutesBorder - 1), | ||
hourFromNow); | ||
} | ||
|
||
[Fact] | ||
public void EighteenHoursBorder() | ||
{ | ||
const int hoursBorder = 18; | ||
VerifyBorder(TimeSpan.FromHours, 18, | ||
GetResource(TimeUnit.Hour, hoursBorder - 1), | ||
dayFromNow); | ||
} | ||
|
||
[Fact] | ||
public void TwentyThreeDaysBorder() | ||
{ | ||
const int monthBorderInDays = 23; | ||
VerifyBorder(TimeSpan.FromDays, monthBorderInDays, | ||
GetResource(TimeUnit.Day, monthBorderInDays - 1), | ||
monthFromNow); | ||
} | ||
|
||
[Fact] | ||
public void TwoHundredAndFourDaysBorder() | ||
{ | ||
const int yearBorderInDays = 274; | ||
VerifyBorder(TimeSpan.FromDays, yearBorderInDays, | ||
GetResource(TimeUnit.Month, 9), | ||
yearFromNow); | ||
} | ||
|
||
private void VerifyBorder(Func<double, TimeSpan> timeSpanFunc, int border, string beforeBorder, string afterBorder) | ||
{ | ||
var date = DateTime.UtcNow; | ||
var date1 = date.Add(timeSpanFunc(border - 1)); | ||
var date2 = date.Add(timeSpanFunc(border)); | ||
var date3 = date.Add(timeSpanFunc(border + 1)); | ||
|
||
var actual1 = _calculator.Calculate(date1, date); | ||
var actual2 = _calculator.Calculate(date2, date); | ||
var actual3 = _calculator.Calculate(date3, date); | ||
|
||
Assert.Equal(beforeBorder, actual1); | ||
Assert.Equal(afterBorder, actual2); | ||
Assert.Equal(afterBorder, actual3); | ||
} | ||
|
||
private static string GetResource(TimeUnit timeUnit, int count) | ||
{ | ||
string resourceKey = ResourceKeys.DateHumanize.GetResourceKey(timeUnit, TimeUnitTense.Future, count); | ||
return Resources.GetResource(resourceKey).FormatWith(count); | ||
} | ||
|
||
private static string GetJustNow() | ||
{ | ||
return Resources.GetResource(ResourceKeys.DateHumanize.Now); | ||
} | ||
} | ||
} |
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.
Oh. Thanks for fixing this :)