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

Add "repeated asyncF evaluation not memoized" law #473

Merged
merged 4 commits into from
Jan 18, 2019

Conversation

kubukoz
Copy link
Member

@kubukoz kubukoz commented Jan 15, 2019

I thought it would be reasonable to check that.

It's easy to break this law, for example by removing the suspend call here: https://github.com/kubukoz/slick-effect/blob/3af6c4ab8950f42ebbcaaed0fc5621c503bb258a/src/main/scala/slickeffect/DBIOAsync.scala#L27. That would memoize the promise, and it would only be completed once.

@codecov-io
Copy link

codecov-io commented Jan 15, 2019

Codecov Report

Merging #473 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #473      +/-   ##
==========================================
+ Coverage    89.6%   89.66%   +0.06%     
==========================================
  Files          70       70              
  Lines        2020     2032      +12     
  Branches      202      197       -5     
==========================================
+ Hits         1810     1822      +12     
  Misses        210      210

Copy link
Member

@alexandru alexandru left a comment

Choose a reason for hiding this comment

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

Note that this law probably breaks binary compatibility for Scala 2.11.

We can live with it for laws, but I'm not seeing Mima rules.

@kubukoz
Copy link
Member Author

kubukoz commented Jan 16, 2019

MiMa after enabling it in laws:

[error]  * method repeatedAsyncFEvaluationNotMemoized(java.lang.Object,scala.Function1)cats.kernel.laws.IsEq in trait cats.effect.laws.AsyncLaws is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.effect.laws.AsyncLaws.repeatedAsyncFEvaluationNotMemoized")
[error]  * method bracketPropagatesTransformerEffects(cats.arrow.FunctionK,java.lang.Object,scala.Function1,scala.Function1)cats.kernel.laws.IsEq in trait cats.effect.laws.BracketLaws is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.effect.laws.BracketLaws.bracketPropagatesTransformerEffects")
[error]  * method bracketTrans(cats.arrow.FunctionK,org.scalacheck.Arbitrary,cats.kernel.Eq,org.scalacheck.Arbitrary,cats.kernel.Eq,org.scalacheck.Arbitrary,org.scalacheck.Arbitrary,org.scalacheck.Arbitrary,org.scalacheck.Cogen,cats.kernel.Eq)org.typelevel.discipline.Laws#RuleSet in trait cats.effect.laws.discipline.BracketTests is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.effect.laws.discipline.BracketTests.bracketTrans")
[error] java.lang.RuntimeException: cats-effect-laws: Binary compatibility check failed!

Is it fine if I add all of them to exclusions?

@alexandru alexandru merged commit b73b1e8 into typelevel:master Jan 18, 2019
@kubukoz kubukoz deleted the asyncf-not-memoized branch March 24, 2019 17:32
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.

5 participants