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

Change to Scala based HOCON config library for JVM #65

Merged
merged 2 commits into from
Dec 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ inThisBuild(
url("https://geirsson.com")
),
scalaVersion := ScalaVersions.head,
crossScalaVersions := ScalaVersions
crossScalaVersions := ScalaVersions,
resolvers += Resolver.sonatypeRepo("snapshots")
)
)

Expand Down Expand Up @@ -83,7 +84,8 @@ lazy val website = project
.dependsOn(
docs,
json,
typesafe
typesafe,
sconfig
)

lazy val core = crossProject(JVMPlatform, JSPlatform)
Expand Down Expand Up @@ -127,6 +129,18 @@ lazy val typesafe = project
)
.dependsOn(coreJVM % "test->test;compile->compile")

lazy val sconfigLib = "org.ekrich" %% "sconfig" % "0.7.0"

lazy val sconfig = project
.in(file("metaconfig-sconfig"))
.settings(
testSettings,
moduleName := "metaconfig-sconfig",
description := "Integration for HOCON using ekrich/sconfig.",
libraryDependencies += sconfigLib
)
.dependsOn(coreJVM % "test->test;compile->compile")

lazy val hocon = crossProject(JVMPlatform, JSPlatform)
.in(file("metaconfig-hocon"))
.settings(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package metaconfig
package sconfig

import org.ekrich.config._
import scala.collection.JavaConverters._
import scala.collection.mutable
import metaconfig.internal.ConfGet

object SConfig2Class {
def gimmeConfFromString(string: String): Configured[Conf] =
gimmeSafeConf(() => ConfigFactory.parseString(string))
def gimmeConfFromFile(file: java.io.File): Configured[Conf] = {
if (!file.exists())
Configured.NotOk(ConfError.fileDoesNotExist(file.getAbsolutePath))
else if (file.isDirectory)
Configured.NotOk(
ConfError.message(s"File ${file.getAbsolutePath} is a directory")
)
else gimmeSafeConf(() => ConfigFactory.parseFile(file))
}
def gimmeConf(config: Config): Configured[Conf] =
gimmeSafeConf(() => config)

private def gimmeSafeConf(config: () => Config): Configured[Conf] = {
val cache = mutable.Map.empty[Input, Array[Int]]
def loop(value: ConfigValue): Conf = {
val conf = value match {
case obj: ConfigObject =>
Conf.Obj(obj.asScala.mapValues(loop).toList)
case lst: ConfigList =>
Conf.Lst(lst.listIterator().asScala.map(loop).toList)
case _ =>
value.unwrapped match {
case x: String => Conf.Str(x)
case x: java.lang.Integer => Conf.Num(BigDecimal(x))
case x: java.lang.Long => Conf.Num(BigDecimal(x))
case x: java.lang.Double => Conf.Num(BigDecimal(x))
case x: java.lang.Boolean => Conf.Bool(x)
case null => Conf.Null()
case x =>
throw new IllegalArgumentException(
s"Unexpected config value $value with unwrapped value $x"
)
}
}
getPositionOpt(value.origin, cache).fold(conf)(conf.withPos)
}
try {
Configured.Ok(loop(config().resolve().root))
} catch {
case e: ConfigException.Parse =>
Configured.NotOk(
ConfError.parseError(getPosition(e.origin, cache), e.getMessage)
)
}
}

private def getPosition(
originOrNull: ConfigOrigin,
cache: mutable.Map[Input, Array[Int]]
): Position =
getPositionOpt(originOrNull, cache).getOrElse(Position.None)

private def getPositionOpt(
originOrNull: ConfigOrigin,
cache: mutable.Map[Input, Array[Int]]
): Option[Position] =
for {
origin <- Option(originOrNull)
url <- Option(origin.url)
linePlus1 <- Option(origin.lineNumber)
line = linePlus1 - 1
input = Input.File(new java.io.File(url.toURI))
offsetByLine = cache.getOrElseUpdate(
input,
ConfGet.getOffsetByLine(input.chars)
)
if line < offsetByLine.length
start = offsetByLine(line)
} yield Position.Range(input, start, start)

}
12 changes: 12 additions & 0 deletions metaconfig-sconfig/src/main/scala/metaconfig/sconfig/package.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package metaconfig

package object sconfig {
implicit val sConfigMetaconfigParser = new MetaconfigParser {
override def fromInput(input: Input): Configured[Conf] = input match {
case Input.File(path, _) =>
SConfig2Class.gimmeConfFromFile(path.toFile)
case els =>
SConfig2Class.gimmeConfFromString(new String(els.chars))
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package metaconfig.sconfig

import metaconfig.Conf
import metaconfig.ConfOps
import metaconfig.ConfShow
import org.scalacheck.Properties
import org.scalameta.logger
import org.scalatest.FunSuite
import scala.meta.testkit.DiffAssertions
import metaconfig.Generators.argConfShow
import org.scalacheck.Prop.forAll

object HoconPrinterProps {
def checkRoundtrip(conf: String): Boolean = {
val a = Conf.parseString(conf).get
val hocon = Conf.printHocon(a)
val b = Conf.parseString(hocon).get
val isEqual = a == b
if (!isEqual) {
pprint.log(a)
pprint.log(b)
logger.elem(conf, hocon, Conf.patch(a, b))
}
a == b
}

}

class HoconPrinterProps extends Properties("HoconPrinter") {
property("roundtrip") = forAll { conf: ConfShow =>
HoconPrinterProps.checkRoundtrip(conf.str)
}
}

class HoconPrinterRoundtripSuite extends FunSuite with DiffAssertions {
def ignore(conf: String): Unit = super.ignore(conf) {}
def checkRoundtrip(conf: String): Unit =
test(conf.take(100)) {
assert(HoconPrinterProps.checkRoundtrip(conf))
}

ignore(
"""
|a.a = "d"
|a.bc = 9
""".stripMargin
)

checkRoundtrip(
"""
|aa.bb = true
|aa.d = 3
|aa.aa = "cb"
""".stripMargin
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package metaconfig.sconfig

import metaconfig.Conf
import metaconfig.ConfOps
import metaconfig.ConfShow
import org.scalacheck.Prop.forAll
import org.scalacheck.Properties
import org.scalameta.logger
import org.scalatest.FunSuite
import scala.meta.testkit.DiffAssertions
import metaconfig.Generators.argConfShow

object PatchProps {
// asserts that applying
def checkPatch(a: String, b: String): Boolean = {
val original = Conf.parseString(a).get
val revised = Conf.parseString(b).get
val patch = Conf.patch(original, revised)
val expected = Conf.applyPatch(original, revised)
val obtained = Conf.applyPatch(original, patch)
if (obtained != expected) {
logger.elem(
obtained,
expected,
patch.toString,
Conf.patch(obtained, expected)
)
}
obtained == expected
}
}

class PatchProps extends Properties("Patch") {

property("roundtrip") = forAll { (a: ConfShow, b: ConfShow) =>
PatchProps.checkPatch(a.str, b.str)
}

}
class PatchPropsSuite extends FunSuite with DiffAssertions {
def check(a: String, b: String): Unit = {
test(a) { assert(PatchProps.checkPatch(a, b)) }
}

check(
"""
|ad.da = true
|cc.bd = "dd"
""".stripMargin,
"""
|
|ad.a.dc = false
|ad = "ad"
""".stripMargin
)

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package metaconfig.sconfig

import java.io.File
import java.nio.file.Files
import java.nio.file.Paths
import metaconfig.Conf
import org.scalatest.FunSuite

class SConfig2ClassTest extends FunSuite {
test("basic") {
val file = File.createTempFile("prefix", ".conf")
Files.write(
Paths.get(file.toURI),
"""|a.b = 2
|a = [
| 1,
| "2"
|]
|a += true""".stripMargin.getBytes()
)
val obtained = SConfig2Class.gimmeConfFromFile(file).get
val expected = Conf.Obj(
"a" -> Conf.Lst(
Conf.Num(1),
Conf.Str("2"),
Conf.Bool(true)
)
)
assert(obtained == expected)
}

test("file not found") {
val f = File.createTempFile("doesnotexist", "conf")
f.delete()
assert(SConfig2Class.gimmeConfFromFile(f).isNotOk)
}

test("null") {
val obtained =
SConfig2Class
.gimmeConfFromString(
"""|keywords = [
| null
|]""".stripMargin
)
.get
val expected = Conf.Obj(
"keywords" -> Conf.Lst(
Conf.Null()
)
)
assert(obtained == expected)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ object TypesafeConfig2Class {
case lst: ConfigList =>
Conf.Lst(lst.listIterator().asScala.map(loop).toList)
case _ =>
value.unwrapped() match {
value.unwrapped match {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't mind this change, but I would expect a drop-in Scala replacement to not require this diff even if that means using () to call non-side-effecting methods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wasn't sure what to do as the internal tests in Scala where inconsistent how it called Java with or without parens. So overall, I tried to make it more Scala-like. I left them on for obvious side-effecting 0-arity methods. Someone who was really bored could do an entire API review 😆

If it really could have been a drop-in then I think Lightbend should have serious considered the PR - it is close but will probably change again for Scala 3 support etc. I also still have to change the package which will require import changes. ☹️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind the parenthesis diff, metaconfig touches such a tiny surface of the API anyways.

internal tests in Scala where inconsistent how it called Java with or without parens.

with or without parens works when calling Java methods, but Scala "nullary methods" have additional information in their signatures to prohibit calling

will probably change again for Scala 3 support

I doubt that ;) I suspect it will work without any changes.

This PR is very close to being ready!

  • change package name so it doesn't conflict with com.typesafe.config
  • new module metaconfig-sconfig to avoid conflict with the old one
  • we can copy-paste the metaconfig-typesafe-config tests for sconfig, I hope to remove the typesafe-config tests at some point so duplication is no problem

case x: String => Conf.Str(x)
case x: java.lang.Integer => Conf.Num(BigDecimal(x))
case x: java.lang.Long => Conf.Num(BigDecimal(x))
Expand All @@ -43,14 +43,14 @@ object TypesafeConfig2Class {
)
}
}
getPositionOpt(value.origin(), cache).fold(conf)(conf.withPos)
getPositionOpt(value.origin, cache).fold(conf)(conf.withPos)
}
try {
Configured.Ok(loop(config().resolve().root()))
Configured.Ok(loop(config().resolve().root))
} catch {
case e: ConfigException.Parse =>
Configured.NotOk(
ConfError.parseError(getPosition(e.origin(), cache), e.getMessage)
ConfError.parseError(getPosition(e.origin, cache), e.getMessage)
)
}
}
Expand All @@ -67,8 +67,8 @@ object TypesafeConfig2Class {
): Option[Position] =
for {
origin <- Option(originOrNull)
url <- Option(origin.url())
linePlus1 <- Option(origin.lineNumber())
url <- Option(origin.url)
linePlus1 <- Option(origin.lineNumber)
line = linePlus1 - 1
input = Input.File(new java.io.File(url.toURI))
offsetByLine = cache.getOrElseUpdate(
Expand Down