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

Remove Order constraints from SortedMap and NonEmptyMap instances #3397

Closed
wants to merge 3 commits into from

Conversation

joroKr21
Copy link
Member

@joroKr21 joroKr21 commented Apr 19, 2020

SortedMap carries around its own instance of Ordering so there is no need for an implicit Order constraint unless we have to create a new SortedMap from scratch (e.g. in Monoid.empty).

Some of the removed constraints were simply not used (e.g. Show, Hash), others were used only in some methods but not others (e.g. Traverse) - note that the standard lib ++ does not require an Ordering.

These inconsistencies are accidental at best and can lead to surprising behaviour at worst (when there are incoherent Order instances in a project).

Closes #3223
Closes #2318

@joroKr21 joroKr21 marked this pull request as draft April 19, 2020 14:45
@joroKr21 joroKr21 mentioned this pull request Apr 19, 2020
@codecov-io
Copy link

Codecov Report

Merging #3397 into master will increase coverage by 0.04%.
The diff coverage is 53.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3397      +/-   ##
==========================================
+ Coverage   92.49%   92.53%   +0.04%     
==========================================
  Files         378      379       +1     
  Lines        7956     7999      +43     
  Branches      227      224       -3     
==========================================
+ Hits         7359     7402      +43     
  Misses        597      597              
Flag Coverage Δ
#scala_version_212 92.56% <53.12%> (?)
#scala_version_213 92.33% <53.12%> (-0.16%) ⬇️
Impacted Files Coverage Δ
core/src/main/scala/cats/Align.scala 96.29% <ø> (ø)
core/src/main/scala/cats/FunctorFilter.scala 100.00% <ø> (ø)
core/src/main/scala/cats/Invariant.scala 92.00% <ø> (ø)
core/src/main/scala/cats/SemigroupK.scala 94.44% <0.00%> (-5.56%) ⬇️
core/src/main/scala/cats/Semigroupal.scala 95.45% <ø> (ø)
core/src/main/scala/cats/Show.scala 68.42% <ø> (+0.85%) ⬆️
core/src/main/scala/cats/UnorderedFoldable.scala 96.42% <ø> (ø)
kernel/src/main/scala/cats/kernel/Eq.scala 84.52% <ø> (ø)
kernel/src/main/scala/cats/kernel/Semigroup.scala 74.64% <0.00%> (-2.17%) ⬇️
...ala/cats/kernel/instances/SortedMapInstances.scala 82.35% <14.28%> (-17.65%) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e02ec0...4ec6d6d. Read the comment docs.

@joroKr21 joroKr21 changed the title [WIP] Remove some SortedMap ordering constraints Remove Order constraints from SortedMap and NonEmptyMap instances Apr 19, 2020
@joroKr21 joroKr21 marked this pull request as ready for review April 19, 2020 20:56
@joroKr21
Copy link
Member Author

@travisbrown I think we should make a decision on this one before 2.2.0 because of bincompat - it modifies directly the new implicit scope instances.

@travisbrown travisbrown added this to the 2.2.0 milestone May 25, 2020
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Looks good to me, just need to fix that one merge conflict. :)

@travisbrown travisbrown self-requested a review June 16, 2020 15:46
if (as.nonEmpty) Option(create(as)) else None

def fromMapUnsafe[K: Order, A](m: SortedMap[K, A]): NonEmptyMap[K, A] =
@deprecated("Use fromMap override without Order", "2.2.0-M2")
Copy link
Contributor

Choose a reason for hiding this comment

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

super nitpick 😄 , now that M2 is out

Suggested change
@deprecated("Use fromMap override without Order", "2.2.0-M2")
@deprecated("Use fromMap override without Order", "2.2.0-M3")

or if it doesn't make it for the M3 RC1 may be. Here and the others

@travisbrown
Copy link
Contributor

The conflict seems pretty simple and I think doesn't require reformatting or anything, so I'll just resolve it here through the web interface if there are no objections (and we can update the since arguments to M3 in a quick follow-up).

@codecov-commenter
Copy link

Codecov Report

Merging #3397 into master will increase coverage by 0.48%.
The diff coverage is 47.36%.

@@            Coverage Diff             @@
##           master    #3397      +/-   ##
==========================================
+ Coverage   91.99%   92.47%   +0.48%     
==========================================
  Files         383      379       -4     
  Lines        8372     8003     -369     
  Branches      212      225      +13     
==========================================
- Hits         7702     7401     -301     
+ Misses        670      602      -68     

@travisbrown
Copy link
Contributor

Oh, looks like I was wrong and it does need a format. @joroKr21, sorry about that—I can grab your commits and open a new PR with them, or you could pull from here and push the format commit?

@joroKr21
Copy link
Member Author

Fine by me 👍

@joroKr21
Copy link
Member Author

Superseded by #3473

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

Successfully merging this pull request may close these issues.

6 participants