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

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

Merged
merged 4 commits into from
Mar 23, 2014

Conversation

wahidshalaly
Copy link
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
Copy link
Member

MehdiK commented Mar 22, 2014

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

@MehdiK
Copy link
Member

MehdiK commented Mar 22, 2014

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
Contributor Author

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


namespace Humanizer.Tests
{
public class DateHumanizeOneTimeUnitTests
Copy link
Member

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
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
Copy link
Member

MehdiK commented Mar 23, 2014

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
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
Copy link
Member

MehdiK commented Mar 24, 2014

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.

/// <param name="format">string format</param>
/// <param name="args">arguments</param>
/// <returns></returns>
public static string FormatWith(this string format, params object[] args)
Copy link
Member

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