These are chat archives for fthomas/refined

29th
May 2017
Torsten Scholak
@tscholak
May 29 2017 00:29
it seems that refined-pureconfig brakes with pureconfig 0.7.1
this works with pureconfig 0.7.0, but fails with 0.7.1:
package pureconfigRegression

import eu.timepit.refined.api.Refined
import eu.timepit.refined.numeric.Positive
import eu.timepit.refined.pureconfig._
import pureconfig._

object main extends App {
  case class Config(positiveInt: Int Refined Positive)

  val config = loadConfig[Config]("pureconfigRegression")
}
Frank S. Thomas
@fthomas
May 29 2017 07:57
Interesting, what's the error?
Viktor Lövgren
@vlovgr
May 29 2017 08:40

There's a compile-time error saying it's not possible to find a PureConfig ConfigReader. PureConfig 0.7.1 breaks binary compatibility because the path parameter of CannotConvert has changed from an Option[String] to a String, where the previous None case is now represented by the empty String. However, fixing this issue, PureConfig no longer seems to pick up the implicit in refined-pureconfig.

There's been some changes in PureConfig 0.7.1 to support deriving readers for value classes, possible that this may be what's breaking it, although our implicit is not even picked up for non-value classes and products with arity > 1.

Mario Pastorelli
@melrief
May 29 2017 08:45
I can have a look tonight if needed
0.7.1 doesn't break the overall derivation and I don't see why it wouldn't pick the readers for a refined types
Viktor Lövgren
@vlovgr
May 29 2017 09:04
I think it might have to do with the Refined type being a value class?
Mario Pastorelli
@melrief
May 29 2017 09:20
Maybe but pureconfig readers should always have lower priority against other readers. If you can't override our behaviour (like you were doing before btw) then we introduced a bug. I need to investigate, it should be easy to check why scalac picks out value class reader over a more specific reader
Viktor Lövgren
@vlovgr
May 29 2017 10:21
True. By the way, is there any reason for using shapeless' Unwrapped instead of Generic for value class derivation? I recently implement similar support for Ciris (https://cir.is) and had all sorts of problems with Unwrapped (especially on older Scala versions). Discovered that Generic supports value classes (whether a case class or not), so simply went for using that instead.
Viktor Lövgren
@vlovgr
May 29 2017 10:26
Ah, I think I remember why: Unwrapped works even if you have a private constructor, while Generic does not.
@ final class DoubleValue private(val value: Double) extends AnyVal
defined class DoubleValue
@ Unwrapped[DoubleValue]
res10: Unwrapped[DoubleValue]{type U = $sess.cmd9.DoubleValue} = shapeless.LowPriorityUnwrappedInstances$$anon$3@29bee3b
@ Generic[DoubleValue]
cmd11.sc:1: could not find implicit value for parameter gen: shapeless.Generic[$sess.cmd9.DoubleValue]
val res11 = Generic[DoubleValue]
                   ^
Compilation Failed
Mario Pastorelli
@melrief
May 29 2017 10:41
Oh I see
We used unwrapped because it looked like a nice addition and I wasn't aware of the issue
With generic you can't understand if it's a value class or no, right?
Viktor Lövgren
@vlovgr
May 29 2017 10:46
I believe not, but I haven't looked into it. It's also quite nice, because you can use the same derivation code regardless of if it's a value class or not. Not sure about any overhead compared to Unwrapped.
Mario Pastorelli
@melrief
May 29 2017 11:04
I would like to read value classes not as objects with one field but as single value by using the reader for the wrapped type
Viktor Lövgren
@vlovgr
May 29 2017 11:12
Unless I'm missing something, it's exactly the same as Generic, except they support different cases.
Torsten Scholak
@tscholak
May 29 2017 11:17
I don't think the fact that my example was a value class is important here. Sorry to have mislead you. As @vlovgr said, it breaks also for more than one parameter, even after the Option[String] fix.
Mario Pastorelli
@melrief
May 29 2017 11:20
Do you have a branch with the fix? I'll have a look today. Except for the value classes derivation, we didn't change anything so my guess is that that particular derivation is the issue
I'm at work now but I'll check this evening
In RefTypeConfigConvertSpec, changing case class Config(value: PosInt) to case class Config(value: Int) doesn't make a difference (still fails). I reverted the value class merge and retried locally, and everything works again, so sounds pretty plausible that it's the cause.
Mario Pastorelli
@melrief
May 29 2017 11:41
All our tests pass, I'm not sure what the issue is but seems a serious regression that we couldn't detect
Mario Pastorelli
@melrief
May 29 2017 17:35
[error] /home/mariop/prog/refined/modules/pureconfig/jvm/src/test/scala/eu/timepit/refined/pureconfig/RefTypeConfigConvertSpec.scala:18: diverging implicit expansion for type pureconfig.ConfigReader[RefTypeConfigConvertSpec.this.PosInt]
[error] starting with method deriveAnyVal in trait DerivedReaders
[error]   ConfigReader[PosInt]
the issue is that the value class reader derivation has the same priority of the one defined in refined
Viktor Lövgren
@vlovgr
May 29 2017 17:45
I guess a good question is whether PureConfig should derive readers/writers for value classes with a private constructor?
Mario Pastorelli
@melrief
May 29 2017 18:44
no, it shouldn't
private constructors should never be used automatically
Viktor Lövgren
@vlovgr
May 29 2017 18:45
Agreed.
Mario Pastorelli
@melrief
May 29 2017 18:50
I'm confused: doesn't Unwrapped require a Generic instance by implementation
oh I see
Mario Pastorelli
@melrief
May 29 2017 18:55
there is an instance for Any that returns the wrapped object
Mario Pastorelli
@melrief
May 29 2017 19:37
ok, fixed. I'll get a review and the publish the next version of pureconfig. I'll let you know when it's online
@tscholak stay tuned
Torsten Scholak
@tscholak
May 29 2017 19:37
thank you so much for your efforts!
it was amazing to see how fast everybody started analyzing and fixing this issue
Mario Pastorelli
@melrief
May 29 2017 19:38
no problem and sorry for breaking the compatibility with this great library. This was unexpected to say the least
Viktor Lövgren
@vlovgr
May 29 2017 19:39
@melrief Thanks a lot for the quick work on getting this fixed! :)
Can confirm patch works with refined-pureconfig.
Torsten Scholak
@tscholak
May 29 2017 19:44
:+1:
Frank S. Thomas
@fthomas
May 29 2017 20:38
@melrief I'm wondering if I need to release a new refined-pureconfig if you could also revert the API change in CannotConvert
Mario Pastorelli
@melrief
May 29 2017 20:40
@fthomas sadly CannotConvert has to stay as String :/. We are moving toward making the API stable, in future this kind of issues will be less frequent.
is it ok @fthomas ?
Frank S. Thomas
@fthomas
May 29 2017 20:46
That's ok. I think people expect that at the least the minor version is changed when you're changing your API like this
Viktor Lövgren
@vlovgr
May 29 2017 20:47
Seeing as PureConfig has had a less stable API than refined since refined-pureconfig's release (and depending on future API stability of PureConfig) it may be worth thinking about moving it to pureconfig-refined (i.e. under the PureConfig repo).
Mario Pastorelli
@melrief
May 29 2017 20:50
@fthomas we will change the minor version next time we break the API, yes. This was a mistake
Frank S. Thomas
@fthomas
May 29 2017 20:53
:+1:
Frank S. Thomas
@fthomas
May 29 2017 21:07
I've no problem with refined-pureconfig being in the refined repo. In this case it would have discovered the bug before the 0.7.1 release. But the same could happen for changes in refined
Derek Morr
@derekmorr
May 29 2017 21:09
the pureconfig maintainers debated internally about 0.7.1 vs 0.8. I guess we should have made the other decision.
Viktor Lövgren
@vlovgr
May 29 2017 21:10
refined PR for pureconfig 0.7.2 is here: fthomas/refined#283
Mario Pastorelli
@melrief
May 29 2017 21:11
PureConfig 0.7.2 has been published, let me know
Derek Morr
@derekmorr
May 29 2017 21:14
i’m glad the two projects were able to work together so quickly
Mario Pastorelli
@melrief
May 29 2017 21:15
same here