Handling of null, blank and untrimmed collection items

[jnm2]

Fixes #555. I'm guessing PublicApiApprovalTest.approve_public_api.approved.txt should be reviewed; are optional params okay or should they be overloads?

[hazzik]

Please add check for expected output, and/or that exception is not thrown.

[jnm2]

Okay. How do I add a check that an exception is not thrown, other than exactly what I have there?

[hazzik]

Assert.DoesNotThrow(()=>...)

[jnm2]

Sure, but I find DoesNotThrow redundant and not in a nice way. The whole premise of its existence flies in the face of how the language itself works.

It's on track to be removed from xunit, and I have to say I agree with the sentiments there: xunit/xunit#188

Other projects like dotnet/roslyn have been removing all usages of DoesNotThrow. I'd hate to perpetuate it. Do I have to?

[hazzik]

Ok. Record.Exception instead of Asser.DoesNotThrow then. I don't care how
it will be implemented, but what I care - is explicitly, that this
particular call to Humanize does not throw
On Tue, 28 Jun 2016 at 9:39 AM, Joseph Musser notifications@github.com
wrote:

In src/Humanizer.Tests.Shared/CollectionHumanizeTests.cs
#566 (comment):

@@ -87,5 +88,58 @@ public void HumanizeUsesObjectFormatterWhenSeparatorIsProvided()
var humanized = _testCollection.Humanize(sc => string.Format("SomeObject #{0} - {1}", sc.SomeInt, sc.SomeString), "or");
Assert.Equal("SomeObject #1 - One, SomeObject #2 - Two, or SomeObject #3 - Three", humanized);
}
+

•    [Fact]
  

•    public void HumanizeHandlesNullItemsWithoutAnException()
  

•    {
  

•        new object[] { null, null }.Humanize();
  

Sure, but I find DoesNotThrow redundant and not in a nice way. The whole
premise of its existence flies in the face of how the language itself works.

It's on track to be removed from xunit, and I have to say I agree with the
sentiments there: xunit/xunit#188
xunit/xunit#188

Other projects like dotnet/roslyn have been removing all usages of
DoesNotThrow. I'd hate to perpetuate it. Do I have to?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Humanizr/Humanizer/pull/566/files/7abd8e2ffe497ad51598278bd8b0c8130f928512#r68661345,
or mute the thread
https://github.com/notifications/unsubscribe/AAI1l8HPS53NAPeZ972HBsFt75gKvNeXks5qQEMegaJpZM4I7H4x
.

[aloisdg]

After reading the xUnit.net's issue, I am in favor to Record.Exception too. Nice reading by the way :)

[jnm2]

I don't believe either addition improves readability- after all, it's a single line in a method named WithoutAnException and we all know how the language works- if anything it's subtly misleading, as though errors outside the DoesNotThrow(() => ) / Record.Exception(() => ) == null ceremony wouldn't be reported, but I'm happy to do whatever matches the project's style.

[hazzik]

are optional params okay or should they be overloads?

I prefer overloads

[jnm2]

I was hoping to keep the API simpler and self-documenting, but I understand the binary compat issue would force that to be a major version update.

[jnm2]

Converted optional params to overloads for minor-version release. Rebased so that you can see, no red in the API.

[hazzik]

Addition of methods to interface is a breaking change (actually any change to an interface is a breaking change): https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/breaking-change-rules.md

[jnm2]

Ah. What are our options then? Extended interface? What would I name it? The extended interface could have optional params.
I hope this can be cleaned up in v3.0.

[hazzik]

@onovotny @mexx can we just skip 2.1 and release this as 3.0?

Also, for future, for 3.0, I would like to replace all interfaces we have with default implementations as the classes are more immune to API changes.

[jnm2]

If 3.0, would you like to see separator or objectFormatter as optional params?

[jnm2]

Hey while we're here, I wonder if anyone has an idea for this? Half the time when I use enumerable.Humanize(), I want to use a verb that needs to be pluralized correctly. (Does/do, is/are, isn't/aren't, exists/exist, etc.)

It's concise to be able to pass a filtered LINQ query to .Humanize(), but not when you have to repeat the query, check if there are fewer or more than one item, and pick the right verb number. Right now the code look like this:

var buffer = enumerable.ToList();
$"{buffer.Humanize()} {(buffer.Count == 1 ? "is" : "are")} ..."

Could there be a more idiomatic way of humanizing "{collection} {verb}", similar to ToQuantity?

[aloisdg]

@jnm2 It will be a lot of work for english, but good luck for i18n. Also you should move this to a new issue. :)

[clairernovotny]

Checking in here, is this ready? I'd be great to get 2.1 out soon.

[jnm2]

@onovotny It depends. I'd like it in as soon as possible, but things get really ugly if we preserve backwards binary compatibility with ICollectionFormatter. If we don't, we're supposed to go to v3.0 which is what @hazzik is asking to do. Thoughts?

[clairernovotny]

I'm not too keen on releasing a 3.0 with just one change like this when there's other things that need to be cleaned up too. Is there really no way to get this without a breaking change? I agree that it'd be good to have default impls to minimize this in the future.

[jnm2]

Alternatives:

1. I don't add StringJoinOptions but the behavior changes to trim items and remove blank items and the version stays in 2.x
2. I do add StringJoinOptions to each ICollectionFormatter method and we go to 3.0- if so, @hazzik would prefer that ICollectionFormatter becomes an abstract class to make versioning easier in the future
3. Version stays 2.x and we add ICollectionFormatterWithOptions : ICollectionFormatter and make a mess and also have to figure out if it's safe to change properties that return ICollectionFormatter to return ICollectionFormatterWithOptions and generic configuration/registry types to <ICollectionFormatterWithOptions>
4. We break the rules of binary back-compat

I feel the same about 3.0. I'm thinking StringJoinOptions could be added in 3.0 but for the time being the default behavior of removing blanks and trimming whitespace would take effect in 2.1. (Option 1) I mean honestly, how often are you going to want to force A, , C , D to happen? If we assume never, maybe StringJoinOptions isn't even a good addition to the API.

[jnm2]

I'm going to rebase again but remove all public API changes so option 1 is ready if that's what you want to go ahead with for 2.1. This doesn't paint us into a corner, unless someone wants to include redundant commas and bad whitespace which I doubt.

[jnm2]

Done, rebased on newest from dev.

[clairernovotny]

@hazzik lmk if this PR looks good to merge in for 2.1 and we can add an issue for 3.0 to refactor the interfaces.