Skip to content

Added groupBy1 and renamed groupByM to groupWhenM #361

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.

Sign up for GitHub

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

Jump to bottom
Merged
larsrh merged 2 commits into from
May 10, 2013
Merged

Added groupBy1 and renamed groupByM to groupWhenM #361

larsrh merged 2 commits into from
May 10, 2013

Conversation

@oxbowlakes
Copy link
Contributor

@oxbowlakes oxbowlakes commented May 8, 2013

Added List.groupBy1 which is like the scala standard library's List.groupBy method except that the returned Map's values are NonEmptyLists. So for example:

List("London", "Munich").groupBy1(_.length) //Map(6 -> NonEmptyList(London, Munich))

This aids in exhaustiveness checks and is more correct.

Additionally, after Lars' suggestion, the commit includes the renaming of groupByM to groupWhenM for consistency with both the standard library and groupWhen

retronym
retronym reviewed May 8, 2013
View reviewed changes
core/src/main/scala/scalaz/std/List.scala Outdated
Copy link
Member

@retronym retronym May 8, 2013

Choose a reason for hiding this comment

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

Can you please deprecate rather than just rename?

Copy link
Contributor

@larsrh larsrh May 8, 2013

Choose a reason for hiding this comment

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

We can deprecate them in 7.0.1, wouldn't that be sufficient?

Copy link
Contributor Author

@oxbowlakes oxbowlakes May 9, 2013

Choose a reason for hiding this comment

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

Given I've spent quite some time over the last few weeks trying to figure out what code changes I need to make in the v7 upgrade, you'll allow me a WAT moment, I'm sure

Copy link
Contributor Author

@oxbowlakes oxbowlakes May 9, 2013

Choose a reason for hiding this comment

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

I've re-added and deprecated (since 7.1)

Copy link
Member

@retronym retronym May 9, 2013

Choose a reason for hiding this comment

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

I think it is better to be binary compatible in the minor releases (e.g 7.0.1), and source compatible w/ deprecation in the major releases (7.1.0). You can't expect everyone to update to the minor releases.

But it's worth checking with the community, and publishing a short compatibility policy for users and committers alike.

Copy link
Contributor Author

@oxbowlakes oxbowlakes May 9, 2013

Choose a reason for hiding this comment

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

I do agree with Jason here - major releases should be source compatible

Copy link
Contributor

@larsrh larsrh May 9, 2013

Choose a reason for hiding this comment

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

Fair enough. In that case, I'll probably roll back #347 and postpone #357.

larsrh added a commit that referenced this pull request May 10, 2013
@larsrh
Merge pull request #361 from oxbowlakes/groupBy1
1718389 
Added groupBy1 and renamed groupByM to groupWhenM
@larsrh larsrh merged commit 1718389 into scalaz:scalaz-seven May 10, 2013
@larsrh
Copy link
Contributor

larsrh commented May 10, 2013

Thank you, Chris!

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

3 participants

@oxbowlakes @larsrh @retronym