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

Fixes #263 Add huminization of collections #268

Closed

Conversation

justin-edwards
Copy link
Contributor

Not ready for merge yet, but I was hoping for a little code review before I went through and documented.

I think it would be useful to have some way to register a default that wasn't ToString() for specific types, though I'm not sure what I'd call it or where that should live or if this PR needs to wait on that feature.

"A Third String",
};

Assert.Equal("A String, Another String, or A Third String", collection.Humanize("or"));
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 May 19, 2014

Thanks. It's very well done.

Just a few comments I left on the code. I also need to check it out in VS. Will try to do tomorrow.


string formatString = count > 2 ? "{0}, {1} {2}" : "{0} {1} {2}";

separator = separator.Trim();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it necessary to trim the provided separator?
Currently there is no test for this line, it could be removed without breaking any present tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd agree, removed in justin-edwards@27583dd


public virtual string Humanize<T>(IEnumerable<T> collection, Func<T, String> objectFormatter, String separator)
{
throw new NotImplementedException("A collection formatter for the current culture has not been implemented yet.");
Copy link
Member

Choose a reason for hiding this comment

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

Haha, cool :)

@MehdiK MehdiK closed this May 21, 2014
@MehdiK
Copy link
Member

MehdiK commented May 21, 2014

Cool. This is rebased and pushed now (because you were a few commits behind). Thanks.

@MehdiK
Copy link
Member

MehdiK commented May 21, 2014

Oh!! I merged and released this and then realized you haven't added the readme entry for it!! Do'h. Can you add the entries please? Soon? :p

@justin-edwards
Copy link
Contributor Author

I'm working on the readme right now.

@MehdiK
Copy link
Member

MehdiK commented May 21, 2014

Cool. Please send it on a separate PR based on top of the latest. Thanks.

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.

3 participants