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 Lazy for Scala 2.13 #660

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cchantep
Copy link
Collaborator

@cchantep cchantep commented Sep 19, 2022

Lazy is not only not required for Scala 2.13, but it raised warnings (see typelevel/doobie#1513 ).

Removing it only for Scala 2.13 using cross version compatiblity.

@cchantep cchantep changed the title TypedEncoder compatiblity Draft: Remove Lazy for Scala 2.13 Sep 19, 2022
@cchantep
Copy link
Collaborator Author

@pomadchin early push for feedback. I plan to apply in other required places in frameless.

@cchantep cchantep marked this pull request as ready for review September 20, 2022 20:21
@cchantep cchantep changed the title Draft: Remove Lazy for Scala 2.13 Remove Lazy for Scala 2.13 Sep 20, 2022
@pomadchin
Copy link
Member

pomadchin commented Sep 20, 2022

@cchantep hmmm but why do we need this change? 🤔 Lazy is not needed in 2.13?

UPD: ooh, i see https://docs.scala-lang.org/sips/byname-implicits.html; should not it be replaced with by-name implicits (i'm just checking scala/scala#6050)?

@cchantep
Copy link
Collaborator Author

@cchantep hmmm but why do we need this change? 🤔 Lazy is not needed in 2.13?

UPD: ooh, i see https://docs.scala-lang.org/sips/byname-implicits.html; should not it be replaced with by-name implicits (i'm just checking scala/scala#6050)?

The tests confirm that what fails without Lazy in Scala 2.13- is working without.

@pomadchin pomadchin self-assigned this Oct 14, 2022
@cchantep cchantep force-pushed the task/scala213-nolazy branch 2 times, most recently from 9a014be to 7caa1ab Compare March 12, 2023 17:41
@codecov
Copy link

codecov bot commented Mar 12, 2023

Codecov Report

Merging #660 (5ab2e3c) into master (a27b04b) will increase coverage by 0.39%.
The diff coverage is 98.56%.

@@            Coverage Diff             @@
##           master     #660      +/-   ##
==========================================
+ Coverage   95.00%   95.39%   +0.39%     
==========================================
  Files          65       71       +6     
  Lines        1180     1281     +101     
  Branches       37       34       -3     
==========================================
+ Hits         1121     1222     +101     
  Misses         59       59              
Flag Coverage Δ
2.12.16 ?
2.12.17 95.00% <97.22%> (?)
2.13.10 95.59% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../src/main/scala/frameless/RecordEncoderField.scala 88.88% <ø> (ø)
...ataset/src/main/scala/frameless/TypedEncoder.scala 98.02% <ø> (+0.62%) ⬆️
dataset/src/main/scala/frameless/ops/As.scala 100.00% <ø> (ø)
...ain/scala-2.13-/frameless/TypedEncoderCompat.scala 96.20% <96.20%> (ø)
...src/main/scala-2.13+/frameless/RecordEncoder.scala 100.00% <100.00%> (ø)
...ain/scala-2.13+/frameless/TypedEncoderCompat.scala 100.00% <100.00%> (ø)
...main/scala-2.13+/frameless/ops/LowPriorityAs.scala 100.00% <100.00%> (ø)
...src/main/scala-2.13-/frameless/RecordEncoder.scala 100.00% <100.00%> (ø)
...main/scala-2.13-/frameless/ops/LowPriorityAs.scala 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cchantep cchantep force-pushed the task/scala213-nolazy branch 2 times, most recently from 72e4cd0 to daba205 Compare March 12, 2023 19:43
@cchantep
Copy link
Collaborator Author

👋 @pomadchin rebased/all ✅

@pomadchin
Copy link
Member

pomadchin commented Mar 14, 2023

@cchantep yo! I'll take a look a bit later 💭

My main concern is that we're adding some code + language specific subdirs just to get rid of warning that can be (?) muted in the client app via a compiler flag. Is the same approach used in circe?

@cchantep
Copy link
Collaborator Author

@cchantep yo! I'll take a look a bit later 💭

My main concern is that we're adding some code + language specific subdirs just to get rid of warning that can be (?) muted in the client app via a compiler flag.

I don't see what you mean.
Once the frameless artifact built for one specific Scala version, there is nothing that can be skipped or disabled on the caller side.

Is the same approach used in circe?

That's how it's done a many project.

@pomadchin
Copy link
Member

pomadchin commented Mar 14, 2023

@cchantep

I don't see what you mean.

How to reproduce the behavior to see what is changed / what we're fighting here?

That's how it's done a many project.

My comment is not about cross scala builds, but about the Lazy usage. Is there a similar problem in i.e. circe codecs derivation (just the first thing that came to my mind with the Lazy usage)?

@pomadchin
Copy link
Member

pomadchin commented Mar 14, 2023

About the compiler flag, i've been referring to the scala/bug#12072 (comment)

@cchantep
Copy link
Collaborator Author

About the compiler flag, i've been referring to the scala/bug#12072 (comment)

That's not specific to this PR and we cannot do anything about.

@pomadchin
Copy link
Member

@cchantep so how to reproduce the behavior to see what is the problem solved in this PR? I haven't faced Lazy being a problem in the other projects with frameless as a dep.

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.

2 participants