Skip to content

Deprecate scala.collection.mutable.MultiMap #8005

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jun 18, 2019

Conversation

joshlemer
Copy link
Contributor

@joshlemer joshlemer commented Apr 26, 2019

In the new Scala-Collection-Contrib module, there exists all three of:

  • scala.collection.MultiDict
  • scala.collection.immutable.MultiDict
  • scala.collection.mutable.MultiDict

They probably even would have been called MultiMap, except that that would cause a name collision with the existing mutable.MultiMap in the standard library.

And so, there is no longer a need for the singular mutable.MultiMap implementation in the core library.

I'd actually prefer that these two data structures (MultiMap/MultiDict and MultiSet/Bag) would be included in core, since they are extremely useful to every day programming. Maybe that can be considered for a future release, but in the meantime we should probably avoid duplicating MultiMap here and there.

Fixes scala/bug#11504

For additional context: scala/scala-collection-contrib#17

@joshlemer joshlemer added release-notes worth highlighting in next release notes library:collections PRs involving changes to the standard collection library labels Apr 26, 2019
@joshlemer joshlemer requested a review from julienrf April 26, 2019 17:45
@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Apr 26, 2019
@julienrf
Copy link
Contributor

I'd actually prefer that these two data structures (MultiMap/MultiDict and MultiSet/Bag) would be included in core, since they are extremely useful to every day programming. Maybe that can be considered for a future release, but in the meantime we should probably avoid duplicating MultiMap here and there.

I don’t see it as an argument to put it to the stdlib. It’s actually useful to have them in a separate module that we can update independently of the standard library.
I also want to add that these collections haven’t been thoroughly tested.

Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merging this, do we need stronger guarantees that collections in scala-collection-contrib are usable replacements for this, and more importantly, that that module is going to be published, eventually?

@lrytz
Copy link
Member

lrytz commented Apr 29, 2019

I'm fine merging this without any further guarantees, as the trait provides very little functionality. Also, I don't see anything that would prevent scala-collection-contrib from being published.

A lighter alternative than a full MultiMap collection would be to turn the methods in the trait being deprecated here into extension methods.

@lrytz lrytz force-pushed the deprecate-multimap branch from 6a960fa to 34f5d3b Compare April 29, 2019 08:01
@lrytz
Copy link
Member

lrytz commented Apr 29, 2019

(pushed a check file update)

@julienrf
Copy link
Contributor

julienrf commented May 2, 2019

Copy link
Contributor

@szeiger szeiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable for 2.13.1.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
library:collections PRs involving changes to the standard collection library release-notes worth highlighting in next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mutable.MultiMap cannot be used as documented without warnings
6 participants