Support scala 2.13

[jilen]

No description provided.

[jilen]

Test case seems not stable.

TestFailedException was thrown during property evaluation.
Message: 271.69345176667715905481568330727 does not match 2716.93451766677159054815683307270
Location: (CustomTypesSpec.scala:41)
Occurred when passed generated values (
      arg0 = 271.69345176667715905481568330727
)

But it runs fine on my local pc

[jilen]

@leonmaia I've make it support scala 2.13 and seems got numeric encoding fixed.
According to https://github.com/postgres-haskell/postgres-wire.

[dangerousben]

Copying in the whole of patchless seems pretty extreme, can't it be updated upstream?

If that's not an option, perhaps rather than copying wholesale we can just provide equivalent functionality using shapeless directly? (I'm not really familiar with the existing factories that Updates provides so I don't know exactly which additional use cases Patch is provided to support).

[jeremyrsmith]

Apologies for neglecting patchless. I've updated its dependencies and published 1.0.7 for Scala 2.11, 2.12, and 2.13. Hope that helps.

[jeremyrsmith]

(FWIW, it's also been so long since I've really worked on finagle-postgres that I don't really remember why it uses patchless)

[dangerousben]

(FWIW, it's also been so long since I've really worked on finagle-postgres that I don't really remember why it uses patchless)

TBH I'd never encountered the feature before, once this is sorted I'll see if I can find some time to add the extra sql dsl features (Columns, Values, Updates) to the guide.

Many thanks for the patchless upgrade :)

[plaflamme]

FWIW: the only place where patchless is used is here

finagle-postgres/finagle-postgres-shapeless/src/main/scala/com/twitter/finagle/postgres/generic/Updates.scala

Lines 36 to 41 in 3d4ab10

	def apply[T, U <: HList, MP <: HList](patch: Patch[T])(implicit
	patchable: Patchable.Aux[T, U],
	mapper: Mapper.Aux[updateWithOptions.type, U, MP],
	toList: ToList[MP, Option[(String, Param[_])]],
	columnNamer: ColumnNamer
	): Updates = apply[U, MP](patch.updates.asInstanceOf[U])

which is provided for convenience.

I would suggest removing it since it can easily be re-introduced in client code that may still want to keep the convenience. Forcing downstream projects to incur a patchless dependency is not worth it just for this convenience method.

[dangerousben]

Apologies for neglecting patchless. I've updated its dependencies and published 1.0.7 for Scala 2.11, 2.12, and 2.13. Hope that helps.

Hi @jeremyrsmith, where's it published to? I can't find 1.0.7 on maven central or bintray.

[dangerousben]

The new patchless is now on maven central. I've pushed a rebased and slightly tidied up version of this PR with the patchless dependency back in. If the tests pass and everyone's happy (which I will assume if nobody says anything), I'll merge.

Removing the patchless dependency altogether is certainly a viable option, but it is a breaking change so I'd rather not personally.

[plaflamme]

Removing the patchless dependency altogether is certainly a viable option, but it is a breaking change so I'd rather not personally.

FWIW: dependency management is hard at scale on the JVM. Dealing with diamond dependency issues or outdated dependencies (i.e.: not using the latest scala version) is a pretty big problem when you write non-trivial projects; I'm sure a lot of people can attest to that.

IMO: Pulling in a dependency (even one that has no dependencies itself) for providing a convenience method is not worth the potential other problems this can cause for users (users that don't even use this method in the first place). A potential way forward to not break (not too much at least) API compatibility is to provide this convenience method in an opt-in subproject, e.g.: finagle-postgres-patchless (similar to how shapeless is an opt-in feature currently).

[jeremyrsmith]

FWIW, I'm almost certainly the one who introduced the patchless dependency and I'd be surprised if anyone really uses it. It looks like the only reason it exists is so that you can use it to make an UPDATE and splice in a Patch[Foo] to describe the update Again, it's been so long since I've worked on finagle-postgres that I don't really remember the motivation, but I'm betting it went something like this – we're going to receive JSON patches in some tiny service, decode them directly to Patch[Foo] values with patchless-circe, and oh boy why can't we just jam those into the database without a bunch of boilerplate. It does seem like an eye-rollingly small reason to introduce a dependency – even if the dependency is small, it doesn't mean it won't cause problems (as evidenced here).

I think adding a deprecation warning when patchless interop is used, and then removing it after another release, would be a perfectly sensible thing to do (and if anyone complains, reintroducing it in another module so that patchless can't hold back progress of the core modules could be a solution).

Quite honestly, the DSL aspect of finagle-postgres-shapeless is pretty half-baked; I only built what I needed at the time which was pretty modest (since nobody else seemed to really be using the library at the time). I certainly wouldn't be offended if the whole DSL were deprecated and people were pointed to something like quill instead for their DSL needs.

[dangerousben]

For my own FWIW, I'm really not bothered either way - at any rate, deprecating or removing it has nothing to do with adding support for scala 2.13 so can happen elsewhere.

(Also FWIW, I quite like the minimal-with-bits-added-as-needed DSL approach, quill is cool and all but sometimes simple is good).