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

CollectionHumanizeExtensions.Humanize causes NullReferenceException for null items #555

Closed
jnm2 opened this issue May 19, 2016 · 7 comments
Labels
Milestone

Comments

@jnm2
Copy link
Contributor

jnm2 commented May 19, 2016

This is a headache for me. .Select(_ => _?.ToString() ?? string.Empty).Humanize() seems unnecessary; wouldn't it be a sensible default to allow .Humanize() directly on collections containing nulls?

Happy to do a PR if you think so.

@clairernovotny
Copy link
Member

Happy to accept a PR to fix this. I would think that there should be a default null value that's overridable if desired. Some people may want String.Empty, but others may want something like Missing or <empty>.

@jnm2
Copy link
Contributor Author

jnm2 commented Jun 1, 2016

Now that I think about it, users might expect enumerable.Humanize() to skip null entries entirely. It isn't very human to display A, , , B, and C. I'd expect each item to be trimmed and null-or-empty items to be skipped so as to avoid the comma. What do you think?

@aloisdg
Copy link
Contributor

aloisdg commented Jun 1, 2016

@jnm2 I agree with your behavior as default if we use a flag to allow something else. We can adapt StringSplitOptions to do the trick:

Member name Description
None The return value includes array elements that contain an empty string
RemoveEmptyEntries The return value does not include array elements that contain an empty string

For Humanize:

Member name Description
None The return value does not includes array elements that contain an empty string
AllowEmptyEntries The return value includes array elements that contain an empty string

Where None is default like in StringSplitOptions.

@jnm2
Copy link
Contributor Author

jnm2 commented Jun 1, 2016

What about this:

[Flags] enum StringJoinOptions
{
    None = 0,
    TrimEntries = 1 << 0,
    RemoveBlankEntries = 1 << 1,
    Default = TrimEntries | RemoveBlankEntries 
}

Blank is synonymous with NullOrWhitespace, whereas Empty is synonymous with == string.Empty.

@jnm2
Copy link
Contributor Author

jnm2 commented Jun 3, 2016

@onovotny Need some direction- how configurable should this be? Don't want to go overboard.

@clairernovotny
Copy link
Member

The options you have there seem ok -- I agree there's no need to go overboard, we can always add more later if we need.

@jnm2
Copy link
Contributor Author

jnm2 commented Jun 3, 2016

You're okay with a default value that would trim all items and remove empty ones? What would the new default mean for back-compat and versioning, or can new[] { "A", "", "B" }.Humanize() -> A, , B be considered a bug?

jnm2 added a commit to jnm2/Humanizer that referenced this issue Jun 13, 2016
jnm2 added a commit to jnm2/Humanizer that referenced this issue Jun 13, 2016
jnm2 added a commit to jnm2/Humanizer that referenced this issue Jun 21, 2016
jnm2 added a commit to jnm2/Humanizer that referenced this issue Jun 21, 2016
jnm2 added a commit to jnm2/Humanizer that referenced this issue Jun 21, 2016
jnm2 added a commit to jnm2/Humanizer that referenced this issue Jun 21, 2016
jnm2 added a commit to jnm2/Humanizer that referenced this issue Jun 21, 2016
jnm2 added a commit to jnm2/Humanizer that referenced this issue Jun 21, 2016
jnm2 added a commit to jnm2/Humanizer that referenced this issue Jun 27, 2016
jnm2 added a commit to jnm2/Humanizer that referenced this issue Jul 3, 2016
@clairernovotny clairernovotny added this to the v2.1 milestone Jul 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants