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

Refactoring Session #1. #104

Closed
wants to merge 1 commit into from
Closed

Conversation

wahidshalaly
Copy link
Contributor

This is an incomplete work. It seems like a big refactor & I think it requires approval for the idea before proceeding.

Please, take your time & review this carefully (but don't merge it).
In this implementation I tried a different approach. Let's call it Dynamic Resource Keys.
So, I don't have constants or fixed names, like current approach, but I've a convention to follow.
This approach is more compact, as you'll see, but you'll lose the benefit of using fixed/constant resource keys.

Let me know what do u think & if you'd like to continue this approach or to keep the current approach.

…ig refactor & I think it requires approval for the idea before proceeding.

namespace Humanizer.Tests.Localisation.DynamicResourceKeys
{
public class ResourceKeyTests
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@MehdiK
Copy link
Member

MehdiK commented Mar 22, 2014

This looks nice on the surface. I am not sure why you duplicated some of the extension classes in a different namespace. This makes it very difficult to see what has changed and also the extensions should live in the root namespace to avoid breaking existing usage.

If you could please change the existing classes, as opposed to adding new ones, this way we can more easily see what has changed and how.

@wahidshalaly
Copy link
Contributor Author

The point, first, I started by refactoring the original implementation then I find out how different it became.
So, I moved the new implementation to a new namespace & returned the original one so you can see them side-by-side & decide which direction do u prefer. As I said this is incomplete & more work to be done especially for localization.

@MehdiK
Copy link
Member

MehdiK commented Mar 22, 2014

I like where this is headed.

To be honest I can more easily see the difference, using a git difftool, if the files are changed. IMO spikes (and more generally PRs) should look like the expected end result so it's easier to see what's changed and how and also to facilitate discussions of different options.

@wahidshalaly
Copy link
Contributor Author

I see this. I tried not to influence the final decision as part of respecting the ownership :)

I've merged recent changes & I'll continue this refactor this weekend & I expect to update the PR by end of tomorrow.

@MehdiK
Copy link
Member

MehdiK commented Mar 22, 2014

superseded with #106

@MehdiK MehdiK closed this Mar 22, 2014
@wahidshalaly wahidshalaly deleted the issue70 branch March 23, 2014 11:56
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.

None yet

2 participants