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

Default collection formatter not throwing exception #394

Closed
wants to merge 3 commits into from
Closed

Default collection formatter not throwing exception #394

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 10, 2015

Regarding issue #392, this implementation of the default collection formatter now does not throw an exception. In my opinion, it is not a good solution to throw an exception in default implementation because this throws an exception on an not supporter language (I had this with German and Danish).

Thomas Mentzel added 3 commits March 10, 2015 19:25
…tes it in regular style with "&" as separator. in already implemented languages, it returns the language specific translation for "and". #392
[Fact]
public void Issue_392_A_collection_formatter_for_the_current_culture_has_not_been_implemented_yet()
{
var originalCulture = Thread.CurrentThread.CurrentCulture;
Copy link
Member

Choose a reason for hiding this comment

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

You can use something like:

using(new AmbientCulture("es")) {
    // test
}

@justin-edwards
Copy link
Contributor

I don't believe this will handle Cyrillic languages, Asian langues, Arabic, or Greek correctly. There may be others I can't think of at the moment as well.

Edit for clarity: I don't know that's enough to prevent going forward with this, just mentioning it.

@hazzik
Copy link
Member

hazzik commented Mar 11, 2015

@justin-edwards what's wrong with Cyrillic languages?

@hazzik
Copy link
Member

hazzik commented Mar 11, 2015

I think DefaultCollectionFormatter should return string without any additional separator, just string.Join(", ", items);

@justin-edwards
Copy link
Contributor

@hazzik, sorry, I had intended to remove Cyrillic languages from that list after I researched it. I may have had bad info on Greek as well (I spoke with someone fluent, but further research contradicts what they said).

@hazzik
Copy link
Member

hazzik commented Mar 19, 2015

@MehdiK, @mexx what'd you say?

@mexx
Copy link
Collaborator

mexx commented Mar 19, 2015

@hazzik I find the idea of & a little better as ,. We already have other areas where the default going to some English inspired implementation. So I'm OK with &.

For me default implementation should produce following output:

new[] { "a", "b" }.Humanize() = "a & b"
new[] { "a", "b", "c", "d" }.Humanize() = "a, b, c & d"

@MehdiK
Copy link
Member

MehdiK commented Mar 20, 2015

I'm with @mexx on this. Also regardless of the default, if one is interested in a better formatter they should just provide their separators in the config.

@MehdiK
Copy link
Member

MehdiK commented Mar 28, 2015

I resolved the raised issues, rebased and pushed the code. Thanks.

@MehdiK MehdiK closed this Mar 28, 2015
@MehdiK MehdiK reopened this Mar 28, 2015
@MehdiK MehdiK closed this Mar 28, 2015
@MehdiK
Copy link
Member

MehdiK commented Mar 29, 2015

This is now released to NuGet as v1.35.0. Thanks for the quick fix.

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.

4 participants