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

Change type of Dict.diff and Dict.intersect #518

Closed
bruno-cadorette opened this Issue Mar 4, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@bruno-cadorette

bruno-cadorette commented Mar 4, 2016

Hello,

It would be nice to change the type of Dict.diff
from diff : Dict comparable v -> Dict comparable v -> Dict comparable v
to diff : Dict comparable a -> Dict comparable b -> Dict comparable a, since the type of the Dict's value is not needed in the implementation.

The same change could be applied to Dict.intersect

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 6, 2016

Contributor

Actually, what your observation suggests to me is that the types should be Dict.diff : Dict comparable v -> Set comparable -> Dict comparable v and Dict.intersect : Dict comparable v -> Set comparable -> Dict comparable v.

That would make for a better API. It would make the intent much clearer, it would make it unnecessary to talk about "preference is given to ..." in the documentation of intersect, and it would cover all the current use cases, as well as yours.

Contributor

jvoigtlaender commented Mar 6, 2016

Actually, what your observation suggests to me is that the types should be Dict.diff : Dict comparable v -> Set comparable -> Dict comparable v and Dict.intersect : Dict comparable v -> Set comparable -> Dict comparable v.

That would make for a better API. It would make the intent much clearer, it would make it unnecessary to talk about "preference is given to ..." in the documentation of intersect, and it would cover all the current use cases, as well as yours.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 6, 2016

Contributor

And probably Dict.diff and Dict.intersect should be renamed as well then, maybe to Dict.drop and Dict.keep. A current call diff dict1 dict2 would become drop dict1 (keys dict2), and similarly for Dict.intersect.

Which raises two further points:

  1. The order of arguments should probably be swapped then, making it Dict.drop : Set comparable -> Dict comparable v -> Dict comparable v (and analogously for Dict.keep).
  2. This depends on Dict.keys returning a Set rather than a List, but that's already on the agenda anyway: https://github.com/elm-lang/core/issues/322
Contributor

jvoigtlaender commented Mar 6, 2016

And probably Dict.diff and Dict.intersect should be renamed as well then, maybe to Dict.drop and Dict.keep. A current call diff dict1 dict2 would become drop dict1 (keys dict2), and similarly for Dict.intersect.

Which raises two further points:

  1. The order of arguments should probably be swapped then, making it Dict.drop : Set comparable -> Dict comparable v -> Dict comparable v (and analogously for Dict.keep).
  2. This depends on Dict.keys returning a Set rather than a List, but that's already on the agenda anyway: https://github.com/elm-lang/core/issues/322
@bruno-cadorette

This comment has been minimized.

Show comment
Hide comment
@bruno-cadorette

bruno-cadorette Mar 6, 2016

I like the names diff and intersect, since it comes from the set theory, it's more intuitive than drop and keep

bruno-cadorette commented Mar 6, 2016

I like the names diff and intersect, since it comes from the set theory, it's more intuitive than drop and keep

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 6, 2016

Contributor

Well, but "diff" and "intersect" don't make sense for things that are not of the same type. In set theory, nobody wants do diff or intersect a set of strings with a set of numbers, for example. So, since your intention is precisely to make the functions available for combining things (dicts) that are not of the same type, it's very inconsistent (IMHO) to stick to those names.

Anyway, the first thing to settle is the API, then the names. My main suggestion is the change of one of the input dicts into a set. Naming comes afterwards.

Contributor

jvoigtlaender commented Mar 6, 2016

Well, but "diff" and "intersect" don't make sense for things that are not of the same type. In set theory, nobody wants do diff or intersect a set of strings with a set of numbers, for example. So, since your intention is precisely to make the functions available for combining things (dicts) that are not of the same type, it's very inconsistent (IMHO) to stick to those names.

Anyway, the first thing to settle is the API, then the names. My main suggestion is the change of one of the input dicts into a set. Naming comes afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment