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

[ base ] Add unambiguous kvList functions for sorted maps #3392

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

buzden
Copy link
Contributor

@buzden buzden commented Sep 23, 2024

Description

Both Data.SortedMap and Data.SortedSet have specialised toList functions.

For Data.SortedSet, I think this is a historical curious case, since its signature and semantics completely copies toList from Prelude, namely from Foldable, which is implemented by SortedSet. I suggest to remove this completely in the future, but suggest now to deprecate this function in favor of prelude's one for backward compatibility.

For Data.SortedMap, toList has different meaning comparing to the one from Foldable, while Foldable is indeed implemented by SortedMap, having Prelude.toList to list all the values, while SortedMap.toList listing all key-value pairs stored in the map. I think having two functions with the same name having different meaning is not a good thing, plus you need to specify namespace each time you use toList for maps (whichever you use). I suggest to introduce a new function for listing key-value pairs list, namely kvList, and to deprecate special key-value toList function in favor of the new one.

To be clear: this PR is a backward-compatible change, but it adds several deprecation warnings, and I suggest the breaking change after the next release (namely, removing deprecated functions).

UPD: I've striked out some outdated text after a bit of discussion and another PR being merged.

Should this change go in the CHANGELOG?

  • If this is a fix, user-facing change, a compiler change, or a new paper
    implementation, I have updated CHANGELOG_NEXT.md (and potentially also
    CONTRIBUTORS.md).

@buzden buzden force-pushed the tokvlist-for-maps branch 2 times, most recently from b8b7bb2 to 3e15034 Compare September 23, 2024 16:36
@dunhamsteve
Copy link
Contributor

There is a fromList which does the opposite of SortedMap.toList. Should we change the name of that (and maybe fromListWith too)?

@buzden
Copy link
Contributor Author

buzden commented Sep 23, 2024

There is a fromList which does the opposite of SortedMap.toList. Should we change the name of that (and maybe fromListWith too)?

I thought about that but 1) I didn't come up with a better name 2) I thought this may be a too much change which would probably be discarded because of the size. For instance, this would change even the Show instance.

@mattpolzin
Copy link
Collaborator

I'm not sure I like the renaming of SortedMap.toList. I like the symmetry that toList/fromList are duals and I find it more useful to consider the action of taking a map to a list to be a key-value list than just a value list so changing the name of SortedMap.toList removes the more intuitive toList in favor of the less intuitive one IMO.

I get the argument that it is unfortunate to have a collision of names with prelude, but I'm not sure I'd vote for resolving it at the cost of the above preferences of mine.

Just my $0.02 though.

I'm not at all opposed to deprecating and removing any specialized implementation that is identical to the generic one (re SortedSet).

@buzden
Copy link
Contributor Author

buzden commented Nov 25, 2024

I find it more useful to consider the action of taking a map to a list to be a key-value list than just a value list so changing the name of SortedMap.toList removes the more intuitive toList in favor of the less intuitive one IMO

Looks like it's okay to make Foldable imlementation to be named in this case. When the current Foldable is named, them toList would behave exactly as you say. However, null function will require explicit named instance instead, that's, probably, not so desired; but we can do the same as with toList, then, we can create a specialised function for this.

I'm not at all opposed to deprecating and removing any specialized implementation that is identical to the generic one (re SortedSet).

I did #3425 for this

@buzden buzden changed the title [ base ] Deprecate toList functions for sorted sets and maps [ base ] Add unambiguous kvList functions for sorted maps Dec 2, 2024
@buzden
Copy link
Contributor Author

buzden commented Dec 2, 2024

After critics above and since #3425 is merged, I've changed this PR to be just an addition of a toList synonym which does not clash with toList from prelude. Feel free to close this PR it this addition looks useless.

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.

3 participants