Escape invalid characters when used in Java identifiers #659
Conversation
|
Oh, also interesting, perhaps: |
Codecov Report
@@ Coverage Diff @@
## master #659 +/- ##
==========================================
+ Coverage 81.13% 81.17% +0.03%
==========================================
Files 78 78
Lines 5163 5195 +32
Branches 130 129 -1
==========================================
+ Hits 4189 4217 +28
- Misses 974 978 +4
Continue to review full report at Codecov.
|
|
Ok, two things:
|
|
Oh -- now Also, apologies for the size of this PR... but I guess most of it is test changes/fixes. |
| @@ -31,30 +31,30 @@ class Issue126 extends AnyFunSuite with Matchers with SwaggerSpecRunner { | |||
|
|
|||
| val handler = q""" | |||
| trait StoreHandler { | |||
| def getRoot(respond: StoreResource.getRootResponse.type)(): scala.concurrent.Future[StoreResource.getRootResponse] | |||
| def getRoot(respond: StoreResource.GetRootResponse.type)(): scala.concurrent.Future[StoreResource.GetRootResponse] | |||
There was a problem hiding this comment.
ah. This is what you meant. Yes, this is quite unfortunate.
modules/sample-dropwizard/src/test/scala/core/Dropwizard/JavaInvalidCharacterEscapingTest.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
One comment, and I request before we merge this that we have a scalafix rule ready to go along with this. I'm happy to write the rule, but I can't get to it this week. If you'd like to write the rule, https://github.com/blast-hardcheese/guardrail-scalafix-rules is not too unpleasant. https://youtu.be/InN_YdVUDnE?t=2507 is where I started writing a scalafix rule for the Tagless Final refactorer if you get stuck, I did everything on-stream.
| for { | ||
| clientName <- formatTypeName(className.lastOption.getOrElse(""), Some("Client")) |
There was a problem hiding this comment.
I think this is a great opportunity to push the string "Client" into each generator, further reducing the impedance mismatch when moving to support non-pascal-case languages.
There was a problem hiding this comment.
This is fine, actually, and i even could have made the param value "client" or "CLIENT" or whatever; the formatter is responsible for doing the right thing for the language. I wanted the generators to not be responsible for naming things, as much as is reasonably possible.
There was a problem hiding this comment.
makes sense, though hopefully my reticence towards hard-coded strings in core is understandable
|
I'll try to take a stab at the scalafix rules, but might not get to it before you have time. |
This is some prep work for adding better identifier escaping to Java, since Java doesn't have something like Scala's backticks for turning an invalid identifier into a valid one. Additionally, the core shouldn't be formatting names of things, since conventions differ between languages. This doesn't completely fix up every usage everywhere, but it should at least remove things like calls to `.toSnakeCase` from the core, and threads through some more pre-made names so the various generator methods don't have to keep rebuilding them.
5569961 to
3e99b31
Compare
This allows characters like `?`, `:`, etc. in things like query and form parameter names, and object schema property names.
3e99b31 to
00a89e9
Compare
|
I'm sure this will need a couple iterations to get right, but here is the scalafix rule I whipped up: guardrail-dev/guardrail-scalafix-rules#1 |
I ran into this when I wanted an enum value called
integer?, and figured I'd add code for escaping all the various non-identifier characters, at least the ones found on a US keyboard (note that$is actually legal in Java identifiers, so it's not included here).This turned out to be a huge rabbit hole, because, despite the work we did last year to make naming better, there was still a lot of inconsistency and hackery around naming things. In order to make this a reasonable problem to solve, I added (to
LanguageTerms) some new methods:formatPackageNameformatTypeNameformatMethodNameformatMethodArgNameformatEnumNameThe bonus here is that as we add other languages with different naming conventions, it becomes trivial to have guardrail follow those naming conventions.
In addition, I set things up so some names (for example, the name of a response class) are generated by the core (by calling the above methods) and threading them into the interpreter methods so the interpreters don't keep rebuilding names in an error-prone way.
This doesn't fix everything. There are still some instances of
.capitalizeand.uncapitalizedhere and there. There are fewer instances than before, which is nice. And the core no longer does.toCamelCase,.toSnakeCase, or.toPascalCaseanywhere. The one thing I am still not able to eradicate isProtocolGenerator'stransformProperty'sneedsCamelSnakeConversion, because doing so still breaks the conflicting-name fixer-upper, and making that continue to work would require more surgery. So, sadly, I will yet again punt on that.This will unfortunately require some user code to change to continue to compile, which is detailed in MIGRATING.
Contributing to Twilio