fix typesafeconfig HOCON include #131
Conversation
metaconfig-typesafe-config/src/main/scala/metaconfig/typesafeconfig/TypesafeConfig2Class.scala
Show resolved
Hide resolved
bjaglin
changed the title
| input <- cache.updateWith(OriginId(origin)) { maybeCurrent => | ||
| maybeCurrent | ||
| .orElse( | ||
| Option(origin.url) |
There was a problem hiding this comment.
this allows to short-circuit the for comprehension when we don't have a URL for the not-seen-before origin, which is the case when a key is defined across files like a in the test
Since df7731d TypesafeConfig2Class was always looking up positions from the main input, causing incorrect mappings at best, or parsing failures when a key was defined in an included file larger than the main one (1) or when a key was defined across files (2). (1) java.lang.IllegalArgumentException: 8 is not a valid line number, allowed [0..1] at metaconfig.Input.lineToOffset(Input.scala:36) at metaconfig.typesafeconfig.Typesafe...(TypesafeConfig2Class.scala:85) (2) java.lang.IllegalArgumentException: -2 is not a valid line number, allowed [0..3] at metaconfig.Input.lineToOffset(Input.scala:36) at metaconfig.typesafeconfig.Typesafe...(TypesafeConfig2Class.scala:85)
| .withLineNumber(-1) // strip potential line suffix | ||
| // use description instead of the nullable filename() or url(), | ||
| // so that we can have an identifier to map to the main input | ||
| // when using ConfigFactory.parseString(string) |
There was a problem hiding this comment.
the complexity of the new implementation comes from gimmeConfFromStringFilename, present in TypesafeConfig2Class but not in Sconfig2Class, and tested via
| @@ -32,7 +32,7 @@ object TypesafeConfig2Class { | |||
| input: Input, | |||
| config: () => Config | |||
| ): Configured[Conf] = { | |||
| val cache = mutable.Map.empty[Input, Array[Int]] | |||
There was a problem hiding this comment.
LGTM nice fix! A lot of pain could have been saved if Scalafmt would have used HOCON idiomatically from the start by supporting include. I resisted supporting include because I had (unfound) concerns it wouldn’t work on JS. In hindsight, it would have been easier to embrace all HOCON features, instead of only a subset.
| maybeCurrent | ||
| .orElse( | ||
| Option(origin.url) | ||
| .map(url => Input.File(new java.io.File(url.toURI))) |
There was a problem hiding this comment.
It’s best to use Input.VirtualFile to avoid hashCode/equals issues when comparing inputs. Input.File should never be used because it has the same hashCode even if the underlying file contents change.
There was a problem hiding this comment.
Good point! I guess in that case it's not a big deal as the Input instances are not used as keys and don't escape private methods? I don't see any factory method for creating a Input.VirtualFile out of a Input.File so I wonder if the extra steps are really worth it here.
There was a problem hiding this comment.
Sounds good, then it should be fine to use Input.File here. I think it would make sense to deprecate Input.File at some point since it's easy to screw up these things resulting in surprising behavior
Originally reported in scalacenter/scalafix#1414. I was surprised it was not first reported through Scalafmt, but it looks like the bump there was recent, so no release was cut with the regression.
Since df7731d
TypesafeConfig2Classwas always looking up positions from the main input, causing incorrect mappings at best, or parsing failures when a key was defined in an included file larger than the main one (1) or when a key was defined across files (2).(1)
(2)