From 01a151afd4a35fe119eb41586dcca2738ddc7a47 Mon Sep 17 00:00:00 2001 From: "Brian J. Tarricone" Date: Wed, 1 May 2019 12:36:26 -0700 Subject: [PATCH] Rewrite case converters and add tests Completely new approach; see comments in the code for a description. --- MIGRATING.md | 25 +++++++ .../guardrail/generators/syntax/package.scala | 73 +++++++++++-------- .../scala/tests/core/CaseConvertersTest.scala | 63 ++++++++++++++++ .../Http4s/RoundTrip/Http4sFormDataTest.scala | 10 +-- 4 files changed, 137 insertions(+), 34 deletions(-) create mode 100644 modules/codegen/src/test/scala/tests/core/CaseConvertersTest.scala diff --git a/MIGRATING.md b/MIGRATING.md index aa57daee03..6b25972ad9 100644 --- a/MIGRATING.md +++ b/MIGRATING.md @@ -1,3 +1,28 @@ +Migrating to 0.49.0 +=================== + +0.49.0 may contain breaking changes for all clients and servers +--------------------------------------------------------------- + +The case converters (the code that takes identifiers and turns them into camelCase, snake_case, etc.) has been rewritten to fix bugs and inconsistencies. As a result, some identifiers (parameter, property, class, etc. names) may be slightly different from what they were before and will need to be updated. + +This is especially true in Scala code, where invalid identifiers were simply backtick-escaped. For example, the following specification: + +``` +parameters: + - name: X-Foo-Bar + in: header + type: string +``` + +which defines a header method parameter, would have generated a method signature such as: + +``` +def someMethod(`x-Foo-Bar`: Option[String]) +``` + +With this change, the parameter name will be the more idiomatic (and backtick-free) `xFooBar`. + Migrating to 0.42.0 =================== diff --git a/modules/codegen/src/main/scala/com/twilio/guardrail/generators/syntax/package.scala b/modules/codegen/src/main/scala/com/twilio/guardrail/generators/syntax/package.scala index 910e2447f8..b948f882e3 100644 --- a/modules/codegen/src/main/scala/com/twilio/guardrail/generators/syntax/package.scala +++ b/modules/codegen/src/main/scala/com/twilio/guardrail/generators/syntax/package.scala @@ -1,5 +1,6 @@ package com.twilio.guardrail.generators +import cats.data.NonEmptyList import java.util.Locale package object syntax { @@ -8,37 +9,51 @@ package object syntax { "Modifications will be overwritten; instead edit the OpenAPI/Swagger spec file." ) - private val toPascalRegexes = List( - "[\\._-]([a-z0-9])".r, // dotted, snake, or dashed case - "\\s+([a-zA-Z])".r, // spaces - "^([a-z])".r // initial letter + /* + * The case converters work as follows. + * + * First we break up the given string into parts. This is a several-stage process + * while we consider boundaries in precedence order. First we split on + * dash/underscore/space/dot, as those are the "strongest" boundary delimiters. + * Then we split on non-uppercase -> uppercase boundaries. We avoid splitting + * just on uppercase because if a part is ALLUPPERCASE then we want to consider it + * a single part. After that, we try to keep runs of uppercase characters (when + * they are followed by non-uppercase characters) in the same group; that is, + * something like "FOOBar" should get broken up into ("foo", "bar"). + * + * There are a few things we just can't accurately handle, like digits. If you + * consider the source string "foo9Bar", there's no way to know if that should + * be grouped as ("foo9", "bar") or ("foo", "9", "bar"). And for "foo9bar", it's + * worse: we don't know if it should be ("foo9bar"), ("foo", "9", "bar"), + * ("foo9", "bar"), or ("foo", "9bar"). In these cases we'll choose to assume + * that digits are not initial group characters. + */ + private val SPLIT_DELIMITERS = "[-_\\s\\.]+".r + private val BOUNDARY_SPLITTERS = List( + "([^A-Z])([A-Z])".r, + "([A-Z]+)([A-Z][^A-Z]+)".r ) implicit class RichString(val s: String) extends AnyVal { - def toPascalCase: String = - toPascalRegexes.foldLeft(s)( - (accum, regex) => regex.replaceAllIn(accum, m => m.group(1).toUpperCase(Locale.US)) - ) - - def toCamelCase: String = { - val fromSnakeOrDashed = - "[-_ ]([a-z])".r.replaceAllIn(s, m => m.group(1).toUpperCase(Locale.US)) - "^([A-Z])".r - .replaceAllIn(fromSnakeOrDashed, m => m.group(1).toLowerCase(Locale.US)) - } - - def toSnakeCase: String = { - val noPascal = "^[A-Z]".r.replaceAllIn(s, _.group(0).toLowerCase(Locale.US)) - val fromCamel = "[A-Z]".r.replaceAllIn(noPascal, "_" + _.group(0)) - fromCamel.replaceAll("[- ]", "_") - } - - def toDashedCase: String = { - val lowercased = - "^([A-Z])".r.replaceAllIn(s, m => m.group(1).toLowerCase(Locale.US)) - "([A-Z])".r - .replaceAllIn(lowercased, m => '-' +: m.group(1).toLowerCase(Locale.US)) - .replaceAllLiterally(" ", "-") - } + private def splitParts(s: String): List[String] = + BOUNDARY_SPLITTERS + .foldLeft(SPLIT_DELIMITERS.split(s))( + (last, splitter) => last.flatMap(part => splitter.replaceAllIn(part, m => m.group(1) + "-" + m.group(2)).split("-")) + ) + .map(_.toLowerCase(Locale.US)) + .toList + + def toPascalCase: String = splitParts(s).map(_.capitalize).mkString + + def toCamelCase: String = + NonEmptyList + .fromList(splitParts(s)) + .fold("")( + parts => parts.head + parts.tail.map(_.capitalize).mkString + ) + + def toSnakeCase: String = splitParts(s).mkString("_") + + def toDashedCase: String = splitParts(s).mkString("-") } } diff --git a/modules/codegen/src/test/scala/tests/core/CaseConvertersTest.scala b/modules/codegen/src/test/scala/tests/core/CaseConvertersTest.scala new file mode 100644 index 0000000000..4b1a1ca667 --- /dev/null +++ b/modules/codegen/src/test/scala/tests/core/CaseConvertersTest.scala @@ -0,0 +1,63 @@ +package tests.core + +import com.twilio.guardrail.generators.syntax.RichString +import org.scalatest.{ FreeSpec, Matchers } + +object CaseConvertersTest { + private case class CaseTest(raw: String, expectedPascal: String, expectedCamel: String, expectedSnake: String, expectedDashed: String) { + override val toString: String = s"""(Test case for: "$raw")""" + } + + private val TEST_CASES = List( + CaseTest("foo", "Foo", "foo", "foo", "foo"), + CaseTest("Foo", "Foo", "foo", "foo", "foo"), + CaseTest("FOO", "Foo", "foo", "foo", "foo"), + CaseTest("fooBar", "FooBar", "fooBar", "foo_bar", "foo-bar"), + CaseTest("fooBarBaz", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("foo-bar-baz", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("foo.bar.baz", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("FooBarBaz", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("Foo-Bar-Baz", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("Foo.Bar-Baz", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("foo-Bar-Baz", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("foo.Bar-Baz", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("Foo-Bar-BazQuux", "FooBarBazQuux", "fooBarBazQuux", "foo_bar_baz_quux", "foo-bar-baz-quux"), + CaseTest("Foo-Bar.BazQuux", "FooBarBazQuux", "fooBarBazQuux", "foo_bar_baz_quux", "foo-bar-baz-quux"), + CaseTest("foo9bar", "Foo9bar", "foo9bar", "foo9bar", "foo9bar"), + CaseTest("foo9Bar", "Foo9Bar", "foo9Bar", "foo9_bar", "foo9-bar"), + CaseTest("9fooBar", "9fooBar", "9fooBar", "9foo_bar", "9foo-bar"), + CaseTest("Foo-_Bar__ baz.", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("FOO BAR BAZ", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("FOO BAR bazQuux", "FooBarBazQuux", "fooBarBazQuux", "foo_bar_baz_quux", "foo-bar-baz-quux"), + CaseTest("FOOBarBaz", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz"), + CaseTest("FooBARBaz", "FooBarBaz", "fooBarBaz", "foo_bar_baz", "foo-bar-baz") + ) +} + +class CaseConvertersTest extends FreeSpec with Matchers { + import CaseConvertersTest._ + + "Pascal case converter should work" in { + TEST_CASES.foreach({ testCase => + withClue(testCase)(testCase.raw.toPascalCase shouldBe testCase.expectedPascal) + }) + } + + "Camel case converter should work" in { + TEST_CASES.foreach({ testCase => + withClue(testCase)(testCase.raw.toCamelCase shouldBe testCase.expectedCamel) + }) + } + + "Snake case converter should work" in { + TEST_CASES.foreach({ testCase => + withClue(testCase)(testCase.raw.toSnakeCase shouldBe testCase.expectedSnake) + }) + } + + "Dashed case converter should work" in { + TEST_CASES.foreach({ testCase => + withClue(testCase)(testCase.raw.toDashedCase shouldBe testCase.expectedDashed) + }) + } +} diff --git a/modules/sample-http4s/src/test/scala/generators/Http4s/RoundTrip/Http4sFormDataTest.scala b/modules/sample-http4s/src/test/scala/generators/Http4s/RoundTrip/Http4sFormDataTest.scala index c4802e6a6b..bef1e0e6ab 100644 --- a/modules/sample-http4s/src/test/scala/generators/Http4s/RoundTrip/Http4sFormDataTest.scala +++ b/modules/sample-http4s/src/test/scala/generators/Http4s/RoundTrip/Http4sFormDataTest.scala @@ -18,7 +18,7 @@ class Http4sFormDataTest extends FunSuite with Matchers with EitherValues { new FooResource[IO]() .routes(new FooHandler[IO] { def doFoo(respond: DoFooResponse.type)(status: sdefs.definitions.Status): IO[DoFooResponse] = - if (status == sdefs.definitions.Status.OK) { + if (status == sdefs.definitions.Status.Ok) { IO.pure(respond.Ok) } else { IO.pure(respond.NotAcceptable) @@ -28,7 +28,7 @@ class Http4sFormDataTest extends FunSuite with Matchers with EitherValues { .orNotFound ) ) - fooClient.doFoo(cdefs.definitions.Status.OK).attempt.unsafeRunSync().right.value shouldBe cdefs.foo.DoFooResponse.Ok + fooClient.doFoo(cdefs.definitions.Status.Ok).attempt.unsafeRunSync().right.value shouldBe cdefs.foo.DoFooResponse.Ok } test("missing required form param") { @@ -36,7 +36,7 @@ class Http4sFormDataTest extends FunSuite with Matchers with EitherValues { new FooResource[IO]() .routes(new FooHandler[IO] { def doFoo(respond: DoFooResponse.type)(status: sdefs.definitions.Status): IO[DoFooResponse] = - if (status == sdefs.definitions.Status.OK) { + if (status == sdefs.definitions.Status.Ok) { IO.pure(respond.Ok) } else { IO.pure(respond.NotAcceptable) @@ -56,7 +56,7 @@ class Http4sFormDataTest extends FunSuite with Matchers with EitherValues { .routes(new FooHandler[IO] { def doFoo(respond: DoFooResponse.type)(status: sdefs.definitions.Status): IO[DoFooResponse] = ??? def doBar(respond: DoBarResponse.type)(status: Option[sdefs.definitions.Status]): IO[DoBarResponse] = - if (status.contains(sdefs.definitions.Status.OK)) { + if (status.contains(sdefs.definitions.Status.Ok)) { IO.pure(respond.Ok) } else { IO.pure(respond.NotAcceptable) @@ -65,7 +65,7 @@ class Http4sFormDataTest extends FunSuite with Matchers with EitherValues { .orNotFound ) ) - fooClient.doBar(Some(cdefs.definitions.Status.OK)).attempt.unsafeRunSync().right.value shouldBe cdefs.foo.DoBarResponse.Ok + fooClient.doBar(Some(cdefs.definitions.Status.Ok)).attempt.unsafeRunSync().right.value shouldBe cdefs.foo.DoBarResponse.Ok } test("missing optional form param") {