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

Break out Vocabulary/DefaultVocabulary to make Inflector more extensible. #408

Closed
wants to merge 4 commits into from
Closed

Break out Vocabulary/DefaultVocabulary to make Inflector more extensible. #408

wants to merge 4 commits into from

Conversation

jkodroff
Copy link

fixes #402

Inflector more exensible.
fixes #402
@jkodroff
Copy link
Author

@MehdiK I made this a little less ambitious than what was discussed in #402. I didn't want to solve anything that wasn't actually being asked for. The idea here is that:

  1. We fixed the immediate problem of the units of measurement.
  2. The user can add additional rules if necessary.

@jkodroff
Copy link
Author

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

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.

@mexx
Copy link
Collaborator

mexx commented Apr 20, 2015

@jkodroff First of all, thank you for the PR.

Regarding the comments:
In Humanizer we try to not break as much as possible and test through the public interface.
You've actually removed the tests for the Singularize and Pluralize inflector extensions, as stated this should be reverted.
The tests for the Vocabulary class should include tests for the new public methods like AddPlural, AddSingular, AddIrregular and AddUncountable. Actually this class is tested anyway through the indirect use from inflector extension methods, so I wouldn't necessarily test it.

Regarding the approval:
Please extend the approval file with the added methods as part of your PR.
The build (build.cmd) shouldn't fail neither on your machine nor on the server.

And also add your PR to the release notes.
Also please add documentation for added possibility of vocabulary customization.

Please see CONTRIBUTING.md for more details.

@jkodroff
Copy link
Author

@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.

@mexx mexx changed the title Break out Vocabulary/DefaultVocabulary to makeInflector more exensible. Break out Vocabulary/DefaultVocabulary to make Inflector more extensible. Apr 26, 2015
@jkodroff
Copy link
Author

jkodroff commented May 7, 2015

Ugh. I'm so sorry for letting this sit so long. I'll be sure to get it
done this weekend.

On Thu, May 7, 2015 at 5:49 AM, Dmytro IELKIN [email protected]
wrote:

@jkodroff https://github.com/jkodroff When do you think you can fix the
tests and commit the change?


Reply to this email directly or view it on GitHub
#408 (comment).

@jkodroff
Copy link
Author

jkodroff commented May 8, 2015

@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() { }
Copy link
Collaborator

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?

@mexx
Copy link
Collaborator

mexx commented May 10, 2015

@jkodroff Looks better.

You've actually removed the tests for the Singularize and Pluralize inflector extensions, as stated this should be reverted.

There were many comments from my side, I think you overlooked this one. Can you please revert the changes to InflectorTests.cs file.

@jkodroff
Copy link
Author

@mexx Are we good now?

MehdiK added a commit that referenced this pull request May 24, 2015
@MehdiK
Copy link
Member

MehdiK commented May 24, 2015

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.

@MehdiK MehdiK closed this May 24, 2015
@MehdiK
Copy link
Member

MehdiK commented May 25, 2015

Thanks for the contribution. This is now available on nuget as v1.36.0.

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.

Add support for excluding certain user-defined words from pluralization / singularization
3 participants