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

Use Typelevel Twiddles #846

Merged
merged 19 commits into from
May 1, 2023
Merged

Use Typelevel Twiddles #846

merged 19 commits into from
May 1, 2023

Conversation

mpilquist
Copy link
Member

@mpilquist mpilquist commented Apr 17, 2023

This PR deprecates left-nested twiddle lists in favor of using Typelevel Twiddles.

Typelevel Twiddles provides a single API that:

  • works on both Scala 3 and Scala 2, allowing cross compilation of client code
  • supports arbitrary arity (i.e. not limited to 22 elements)
  • provides seamless support for regular tuples on Scala 3 and compatibility with regular tuples on Scala 2

As a result, codecs/encoders/decoders are now composed via *: instead of ~ and gimap/gmap/gcontramap have been replaced with as.

The ~ operation is deprecated and will be removed in Skunk 1.0.

Feats of strength were performed to maintain source compatibility. Source breakage was unavoidable in the case of parameterized commands, which now return commands of new style twiddle lists. See the docs page on Twiddle Lists for migration, including the option of adding import skunk.feature.legacyCommandSyntax to (temporarily) restore the old behavior.

@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2023

Codecov Report

Merging #846 (a6cae22) into main (570ae54) will decrease coverage by 1.24%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##             main     #846      +/-   ##
==========================================
- Coverage   85.11%   83.88%   -1.24%     
==========================================
  Files         126      126              
  Lines        1747     1756       +9     
  Branches      147      155       +8     
==========================================
- Hits         1487     1473      -14     
- Misses        260      283      +23     
Impacted Files Coverage Δ
...red/src/main/scala-2/syntax/StringContextOps.scala 90.00% <ø> (ø)
...s/core/shared/src/main/scala-2/util/Twiddler.scala 0.00% <ø> (-100.00%) ⬇️
modules/core/shared/src/main/scala/Codec.scala 96.96% <ø> (-3.04%) ⬇️
modules/core/shared/src/main/scala/Decoder.scala 92.00% <ø> (-4.00%) ⬇️
modules/core/shared/src/main/scala/Encoder.scala 93.33% <ø> (-3.34%) ⬇️
modules/core/shared/src/main/scala/Fragment.scala 78.26% <ø> (-4.35%) ⬇️
modules/core/shared/src/main/scala/Session.scala 57.30% <0.00%> (-4.15%) ⬇️
modules/core/shared/src/main/scala/package.scala 66.66% <ø> (+33.33%) ⬆️
...ules/core/shared/src/main/scala/syntax/IdOps.scala 0.00% <ø> (-100.00%) ⬇️
...s/core/shared/src/main/scala/AppliedFragment.scala 100.00% <100.00%> (ø)
... and 3 more

@mpilquist
Copy link
Member Author

Any thoughts on when (if?) we should deprecate legacy twiddle lists (i.e. the left nested tuples). My initial thought was to deprecate them in the 0.6 release and remove them entirely in 1.0. However, parameterized commands make that gradual cutover difficult. Consider:

  val insertCity2: Command[City] =
    sql"""
        INSERT INTO city
        VALUES ($int4, $varchar, ${bpchar(3)}, $varchar, $int4)
      """.command.contramap {
            case c => c.id ~ c.name ~ c.code ~ c.district ~ c.pop
          }

The sql interpolator uses ~ for params. If we change that to use *: instead, we break all usage like the above.

@chuwy
Copy link
Contributor

chuwy commented Apr 27, 2023

I humbly vote for deprecating them ASAP. The *: / ~ ambiguity caused lots of problems in my derivation code (I have such thing in my private lib).

@mpilquist
Copy link
Member Author

mpilquist commented Apr 27, 2023

One option that I haven't fully thought through is adding an import that opts-in to the 1.0 syntax.

For example, without an explicit import, this could would continue to work without change:

  val insertCity2: Command[City] =
    sql"""
        INSERT INTO city
        VALUES ($int4, $varchar, ${bpchar(3)}, $varchar, $int4)
      """.command.contramap {
            c => c.id ~ c.name ~ c.code ~ c.district ~ c.pop
          }

But if you added import skunk.syntax.future, then you'd get to write:

  // On Scala 3 only
  val insertCity2: Command[City] =
    sql"""
        INSERT INTO city
        VALUES ($int4, $varchar, ${bpchar(3)}, $varchar, $int4)
      """.command.contramap {
            c => (c.id, c.name, c.code, c.district, c.pop)
          }

  // On Scala 2 or Scala2+3 cross build
  val insertCity2: Command[City] =
    sql"""
        INSERT INTO city
        VALUES ($int4, $varchar, ${bpchar(3)}, $varchar, $int4)
      """.command.contramap {
            c => c.id *: c.name *: c.code *: c.district *: c.pop *: EmptyTuple
          }

@michael-smith
Copy link

One option that I haven't fully thought through is adding an import that opts-in to the 1.0 syntax.

Never really been a fan of the future opt-in import. I agree with @chuwy that it should just be dealt with ASAP.

@mpilquist
Copy link
Member Author

Commands now default to the new twiddle syntax. Adding import skunk.feature.legacyCommandSyntax results in commands using left nested tuples.

@mpilquist mpilquist marked this pull request as ready for review April 30, 2023 14:22
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