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

Date precision #133

Closed
wants to merge 3 commits into from
Closed

Date precision #133

wants to merge 3 commits into from

Conversation

MehdiK
Copy link
Member

@MehdiK MehdiK commented Apr 7, 2014

Slightly based on #114 to implement #101

WIP - there are still a few failing tests for precision based tests; but want to get some code review on this first.

@wahidshalaly
Copy link
Contributor

Now I'm confused! You mentioned before that you don't like the idea of having different strategies/calculators & you prefer to have only the precision-based implementation. But while reviewing this I found that you renamed IDistanceOfTimeInWords interface to be IDateTimeHumanizeStrategy ?! Can you clarify more what is our goal now?

@MehdiK
Copy link
Member Author

MehdiK commented Apr 9, 2014

Hahaha, yeah that's what I meant by the Blonde Moment in my comments, because I thought the default one was breaking at .75 precision and I was proven wrong later when I went through the tests. Now I know the default strategy logic is badly broken as it has some weird inconsistent break points.

We have two options here: we can either drop the current implementation and replace it with precision based with precision set to 1, which is not too bad, or keep the default one in there for backward compatibility perhaps and add the precision based in as a second algorithm. I like the first option better; but breaking the logic might lead into unexpected results for the existing users which is why I think having strategies in place is good, at least until we deprecate the default one.

@wahidshalaly
Copy link
Contributor

"until we deprecate the default one" this sounds good to me too.
But I see that you replaced the precision-based implementation too? any problem with it?
Let me tell you intention behind my algorithm/implementation.

It works out approximation on all time parts/units, from the smaller to the bigger, then it calculates the final result based on the biggest unit.

For example, if it's a year & 8 months & 23 days .. then

  1. 23 days are approx. one month
  2. Add one month to 8 months = 9 months approx one year
  3. Add one year, then the final result is 2 years.
    So I'm not applying the .75 directly (or blindly) on number of days.
    I think this gives the calculation a human-like approximation :)

@MehdiK
Copy link
Member Author

MehdiK commented Apr 9, 2014

That's something I am trying to implement here too (still incomplete); but your algorithm is quite superior.

I took your code and added a few tests on top and some of them failed; but I think your approach is much better. So I think we should try to fix it for the breaking tests and use that instead. If you are happy with the rest of this PR, do you think you can grab this, change the algorithm to use yours and fix the tests? That's so you get the credit for your awesome work and not me :)

Also please add in a test for the case you gave above.

If you have other issues with this PR please let's discuss them.

@wahidshalaly
Copy link
Contributor

Glad that you like the algorithm.. & Thanks for the credit :)

The question now.. Why are we starting on a new PR? Why don't we rebase the original PR on top of the current master? What will I miss? or what do you like to change from this point? I guess it should be easier or what?!

@MehdiK
Copy link
Member Author

MehdiK commented Apr 10, 2014

A few things made me create a new PR:

  • I tried REALLY hard to rebase that on top of master and it was rather impossible. So many conflicting changes.
  • From my reviews on your PR "I am not sold on DistanceOfTime class and the new Humanize method on IFormatter (and unification of DateTime and TimeSpan formatters)". I actually made these changes on your PR too but never pushed up because couldn't rebase.
  • And there were a few things I avoided on this PR; e.g. No DefaultPrecision on Configurator, no TimeStructure, no Humanize method on formatter etc.

@wahidshalaly
Copy link
Contributor

Okay. So I need to bring over the algorithm implementation & fix broken tests using current structure. Right?

@MehdiK
Copy link
Member Author

MehdiK commented Apr 10, 2014

Yes please.

@wahidshalaly
Copy link
Contributor

I'm fixing tests now. The reason they're broken is the wrong assumption.

Given the latest algorithm we talked about ..
Both 89 & 90 seconds => one minute.. starting form 105 seconds => 2 minutes
Because 105 seconds = 1 minutes & 45 seconds (approx. 1 minute) = 2 minutes
while 90 seconds = 1 minute & 30 seconds (< 75% of a minute) so it's ignored.
The approximation rule is (< 0.75 )=> 0 & (>= .75) => 1
For 90 seconds to be 2 minutes, you need precision of 0.5

Agreed? do you like to change something about the precision calculation?

@MehdiK
Copy link
Member Author

MehdiK commented Apr 10, 2014

I guess you might be looking at an old version of the PR. I had fixed the tests. Well at least I hope - unless I dreamt the whole thing!! #bites_finger #checking_now

@wahidshalaly
Copy link
Contributor

I hope not 😟 but I see the TeamCity Build also broken for unit tests!
I'm tracking upstream/date-precision branch (not a PR branch)

@MehdiK
Copy link
Member Author

MehdiK commented Apr 10, 2014

No, I am sure they're fixed. Just git pull upstream date-precision and you should see the latest. Can verify it online here

@wahidshalaly
Copy link
Contributor

2014-04-10_2106
This conversation is located on PR/133 !!

@MehdiK
Copy link
Member Author

MehdiK commented Apr 10, 2014

The unit tests are broken because I never completed the algorithm as we're going to use yours.

Wanna pop on Skype? I'm www.mehdi-khalili.com

@MehdiK
Copy link
Member Author

MehdiK commented Apr 10, 2014

Superseded by #165

@MehdiK MehdiK closed this Apr 10, 2014
@MehdiK MehdiK deleted the date-precision branch April 20, 2014 15:13
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.

2 participants