HashSet constructor IEnumerable / fixing distinct check

[StefanBertels]

Hi,

1. I think there is a bug in internal constructor for Set/HashSet with checkUniqueness. You pass this to tryAdd. I think tryAdd will ignore duplicates, so probably a negation missing. This commit contains a fix.
   Seems you use this constructor only for Bind when creating new sets. I changed the name of checkUniqueness to tryAdd and negated the values there. IMHO this makes it more clear because everything is now "tryAdd". But perhaps you don't like this way... then you probably have to add "!" in constructor code before calling Internal constructor like new SetInternal<OrdDefault<A>, A>(items, !checkForUniqueness) to just fix the bug.

2. I splitted HashSet constructor function the same way it is splitted for Set. I didn't really verify this but I guess the errors I get when deserializing json are due to a "missing" constructor with only IEnumerable<T> as arg. (It's working for Set.)
   At least this makes Set and HashSet same style.

Side note: I switched my code from Set to HashSet because I noticed some "strange" behaviour when comparing my sets. The cause was that I have Sets of a Record<T> with [OptOutOfEq] and custom Equals -- but Set equality comparison (and especially Contains) uses Ord. This is not a bug in LanguageExt, but reminded me that Set can only be used with types that have valid Ordering. I just need Equality for my use case so that's all I want to implement.

[StefanBertels]

Ah, an important note: My patch changes behaviour of HashSet! By default duplicates will just be ignored now. I just replicated behaviour of Set. Maybe you want to raise errors for duplicates by default?

Anyway: I would expect same behaviour for HashSet and Set.

[StefanBertels]

Some more thoughts on general design regarding HashSet/Set and duplicates. (Note: I think HashSet and Set should behave same way. Maybe this is related to HashMap, too.)

There are some options to create a Set like:

• Map
• Bind
• IEnumerable constructor

Should Map, Bind and constructor use Add() or TryAdd()?

I personally like the more strict way to use Add because this will uncover errors instead of silently dropping elements. AFAIK you use this in Bind and Map.

Construction Set using an IEnumerable is similar. We could be strict here, too. But this would in fact result in uncovering errors or forcing the caller to use Distinct() before. The latter isn't possible if IEnumerable constructor is used by some other libraries for deserialization (json). So you would force to have the input to be distinct. This would mean: use Add everywhere (would change Set constructor behaviour).

But you could see it the other way, too: If data type is something like Set then it's likely that the caller would want to implicitely Distinct here -- if he doesn't want the operation to validate input. I think validating input wouldn't be best style here and I guess there are many use cases where it would be a nice shortcut to just call Map/Bind/constructor with implicit Distinct functionality. You might see this as something similar as GroupBy. Example: you map (a set of) vcard entries to a Set of city names by giving _ => _.City as mapper function. Just a thought.

If Map/Bind are strict and constructor is not (current behaviour for Set) comments should make this clear and other functions should be checked for consistency (toSet(...), Set(1,2,3,1)).

Just some thoughts. What do you think? Anyway we should add some test cases to make sure behaviour is consistent and doesn't accidentally change later. Let me know if I should write some.

[louthy]

I think too much has changed since this. So, if you still feel strongly, please resub