Issue #101: Draft for precision-based calculator #114
Conversation
|
Could you please rebase this with the latest stuff? Also I have done an Api Approver test on Humanizer which has led to one breaking test on your PR. Would appreciate if you could fix it. |
|
I've pulled latest, rebased, and pushed again. Waiting for CI result. |
|
Sorry. I closed this by mistake! I've reopened it again. |
|
Still got the failing api approver test. Can you please run the tests locally, update the approved file and push a new commit to fix the test? |
…ze() method very compact & also give us chance to introduce a new way of calculation, precision-based calculation. By this we can have a non-breaking change since that it's optional to choose between the default (old) calculator & the precision-based (new) calculator. Also It'll be open for everyone to have his own calculation if there's a special need.
Duplicated DateHumanizeTests, as PrecisionBasedDistanceOfTimeTests, to examine compatibility with previous calculation method. Minor refactor: replaced Dotiw by DistanceOfTime in namespaces & types.
|
Fixed. I didn't know about PublicApiApprovalTest. It was ignored before. |
|
It's from ApprovalTests. We have an approved public facing API documentation and if you change the API in any way the test will fail and will show you a diff tool with the changes you've made to the API. Thanks for updating the approved file & fixing the test. I will have a look soon. |
| @@ -22,7 +22,8 @@ public AmbientCulture(string cultureName) | |||
|
|
|||
| public void Dispose() | |||
| { | |||
| Thread.CurrentThread.CurrentUICulture = _culture; | |||
| Thread.CurrentThread.CurrentCulture = _callerCulture; | |||
|
There are things I quite like in this PR; but I am not sold on |
|
|
||
| namespace Humanizer.Tests | ||
| { | ||
| public abstract class DateHumanizeTestsBase : IDisposable |
There was a problem hiding this comment.
It's good this is extracted out as we need this in #123.
|
As per my last note, I think this PR could be simplified down to replacing the old algorithm with the new precision based one. I guess, and I don't like this as the param list is growing, the What do you think? |
|
Sorry mate. I must've smoked my breakfast when I reviewed this PR, and that 'aha moment' should be read 'blonde moment' ;) |
|
Hey mate, I tried to rebase your work on top of upstream and apply the changes I was talking about but it proved too difficult and also what I had in mind was a bit different from your implementation. There were also a few corner cases that your precision based algorithm didn't cater for; e.g. '1500 milliseconds -> now' while it should return 'one second'. So I created #133 slightly based on your implementation. Please check it out and let me know what you think. |
|
Superseded by #165 |
This was an interesting issue. The point what makes me don't feel free while implementing this is the static extension method Humanize(). I understand its value but I think it has side effects. Sure, this is not final but I'll stop until I get your feedback.
Note: There's a proposal to how to unify DateHumanize() & TimeSpanHumanize() in DefaultFormatter. Tell me what do you think.