-
Notifications
You must be signed in to change notification settings - Fork 966
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
Break out Vocabulary/DefaultVocabulary to make Inflector more extensible. #408
Break out Vocabulary/DefaultVocabulary to make Inflector more extensible. #408
Conversation
Inflector more exensible. fixes #402
Assuming that the API difference failures are something you'll have to approve. |
|
||
namespace Humanizer | ||
{ | ||
/// <summary> | ||
/// Provides hint for Humanizer as to whether a word is singular, plural or with unknown plurality | ||
/// </summary> | ||
public enum Plurality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkodroff what is the reason why you moved this enum? Please move it back to Humanizer
namespace.
@jkodroff First of all, thank you for the PR. Regarding the comments: Regarding the approval: And also add your PR to the release notes. Please see CONTRIBUTING.md for more details. |
@mexx That all sounds reasonable to me. I think I should be able to find time to make those changes this week. Thanks for your patience with this new contributor - PR's on well-documented libraries like this one are new to me. |
Ugh. I'm so sorry for letting this sit so long. I'll be sure to get it On Thu, May 7, 2015 at 5:49 AM, Dmytro IELKIN notifications@github.com
|
@mexx Made the requested changes. We should be good now. If you want to kill this PR and have me resubmit these changes squashed as a single commit, no problem. |
@@ -243,6 +243,23 @@ public class In | |||
public System.DateTime TheYear(int year) { } | |||
} | |||
|
|||
public class Vocabularies | |||
{ | |||
public Vocabularies() { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please remove this default constructor from public surface?
This class isn't intended to have any instance, right?
@jkodroff Looks better.
There were many comments from my side, I think you overlooked this one. Can you please revert the changes to |
@mexx Are we good now? |
Thanks @jkodroff. I made a few changes, rebased your branch on top of origin and pushed up. Please have a look at the changes I made to see if you're happy with them. |
Thanks for the contribution. This is now available on nuget as v1.36.0. |
fixes #402