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

[#17] Rename MultiSet to Bag #24

Closed
wants to merge 3 commits into from

Conversation

joshlemer
Copy link
Contributor

@joshlemer joshlemer commented Jul 3, 2019

#17
Other changes:

  • A bit more elaboration in the scaladocs explaining what a collection.Bag is.
  • MultiMapTest /SortedMultiMapTest etc are renamed to MultiDictTest etc (must have forgotten to rename these when we went from MultiMap->MultiDict)

@exoego
Copy link

exoego commented Jul 24, 2019

Could you consider adding aliases something like

@deprecated("Use Bag instead", "0.1.1")
type MultiSet = Bag

so existing users can notice deprecation and consider migration.

@joshlemer
Copy link
Contributor Author

@exoego if I were to implement that with package objects (because types cannot be at top level) then I would run into conflicts of package objects with the canonical scala.collection scala.collection.immutable and scala.collection.mutable versions wouldn't I? I get compile errors when I try to implement the scala.collection.immutable package object here.

@joshlemer
Copy link
Contributor Author

@exoego @julienrf any idea about the above comment? I don't know how it would be possible to implement these type aliases without conflicting with other package objects in the stdlib proper.

@julienrf
Copy link
Collaborator

I don’t see a nice solution to this problem. Would that work to define a deprecated class?

@deprecated("Use Bag instead", "0.1.1")
class MultiSet[A] extends Bag[A]

@joshlemer
Copy link
Contributor Author

At that point, maybe just leave in the MultiSet implementation, as deprecated, and include the new Bag implementation separately?

@exoego
Copy link

exoego commented Oct 25, 2019

You can define type aliases in package object.
So type alias looks top-level.

@exoego
Copy link

exoego commented Oct 25, 2019

ah sorry, you already mentioned conflict in that approach

@exoego
Copy link

exoego commented Oct 26, 2019

How about this change? joshlemer#1

@joshlemer
Copy link
Contributor Author

@exoego that is the change that I tried to make but it resulted in compile errors, I'm pretty sure because of the duplicate package objects, but I'll merge that in for now and see how the CI handles it.

Preserve deprecated MultiSet as type alias of Bag
@SethTisue
Copy link
Member

@joshlemer no activity in a while — should this stay open?

@SethTisue SethTisue marked this pull request as draft October 27, 2020 03:08
@joshlemer
Copy link
Contributor Author

I ended up being blocked by #64 (and I think still would be) and so wasn't able to work on this any longer, but I still think Bag is a better name and worth switching to

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.

4 participants