Skip to content

Issue #70 - Replacing current static resource keys with new dynamic keys#106

Merged
MehdiK merged 4 commits into
Humanizr:masterfrom
wahidshalaly:feature/issue70
Mar 23, 2014
Merged

Issue #70 - Replacing current static resource keys with new dynamic keys#106
MehdiK merged 4 commits into
Humanizr:masterfrom
wahidshalaly:feature/issue70

Conversation

@wahidshalaly

Copy link
Copy Markdown
Contributor

In this pull request, I have merge recent changes from original repository and completed refactoring we started last time.

…ig refactor & I think it requires approval for the idea before proceeding.
Conflicts:
	src/Humanizer.Tests/DateHumanizeTests.cs
@MehdiK MehdiK mentioned this pull request Mar 22, 2014
@MehdiK

MehdiK commented Mar 22, 2014

Copy link
Copy Markdown
Collaborator

I like where you're going with this and I think this is cleaner than the current implementation.

@MehdiK

MehdiK commented Mar 22, 2014

Copy link
Copy Markdown
Collaborator

Could you take out all the Dynamic folders and namespaces? I don't think we need to let the implementation details show up in the folder structure or namespace. Perhaps as discussed in email, close this one and send a new PR with latest changes.

@wahidshalaly

Copy link
Copy Markdown
Contributor Author

TeamCity is down!! I'll check failing build as soon as it's build server is back online.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we need these tests? These are already verified in DateHumanizeTests.

@wahidshalaly

Copy link
Copy Markdown
Contributor Author

Those were introduced in previous time to verify the new way of resource key generation. They don't have much value at the moment so you can decide either to leave or to delete them.

@MehdiK MehdiK merged commit d0f4b8a into Humanizr:master Mar 23, 2014
@MehdiK

MehdiK commented Mar 23, 2014

Copy link
Copy Markdown
Collaborator

I made a few changes and also refactored IFormatter, DefaultFormatter and TimeSpan.Humanize. Please check it out and let me know what you think.

@wahidshalaly wahidshalaly deleted the feature/issue70 branch March 23, 2014 11:55
@wahidshalaly

Copy link
Copy Markdown
Contributor Author

I like the _TimeSpanFormatter_ refactoring, it's just the very logical next step 👍
I'm not very sure about _TimeUnitTense, for me _isFuture flag was good enough (maximum we can use _isFuture: true_ instead of _true_ only).

I think this enough refactoring for Date.Humanize() at the moment we better move to precision issue which again will require thinking about Date.Humanize but in a new context.

I'm thinking if we can introduce a different strategies that would be better.
For example, introduce IDistanceOfTimeInWords and the first default implementation is the current calculation & the next implementation is the one that's based on precision by this the change won't be very breaking :) In other words, how would like to use the new precision option can deliberately use the different strategy otherwise he won't be affected by change. The difficult point that requires thinking is how are we going to do so while we're using extension methods!

@MehdiK

MehdiK commented Mar 24, 2014

Copy link
Copy Markdown
Collaborator

I was a bit unsure about TimeUnitTense too, but I try to stay away from boolean flags as much as possible. Maybe in this case it's an overkill though. Will compare the implementations again. Thanks.

I don't think there's a need for strategy for implementing precision. It should be relatively easy; but I think the DateTime.Humanize still needs refactoring, definitely after implementing precision. At the end TimeSpan.Humanize and DateTime.Humanize should look as consistent as possible. There is also a precision in TimeSpan.Humanize but it plays a different role.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is nice. I don't know how this skipped under my radar. Please add unit tests for it and also add it to the readme with an entry on the table of content (in a new PR).

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