Avoid generating invalid Scala identifiers when printing schemas and protocols #178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There is already a `normalize` function which makes an attempt to normalize strings into valid Scala identifiers, but it does not handle all cases correctly, e.g. it does not handle reserved words. Added an `ident` function which does a better job at converting an arbitrary string into a valid identifier, by delegating the logic to scalameta. Rather than fiddling with the contents of the string to make it valid, we just wrap it in backticks if required. Unfortunately, `normalize` is used for a lot of different things, not only generation of valid identifiers, so I then had to go through the code and switch from `normalize` to `ident` in *just* the right places to make the tests pass. The codegen behaviour has changed slightly (for the better I think) in a few cases. See the updated unit tests for the details.
Similar to the previous commit, just wrapping strings in `toValidIdentifier` in all the right places. Extended the existing protobuf printing test to cover this new behaviour.
juanpedromoreno
approved these changes
Dec 5, 2019
| */ | ||
| def toValidIdentifier(string: String): String = | ||
| if (string.isEmpty) string | ||
| else scala.meta.Term.Name(string).syntax |
Co-Authored-By: Juan Pedro Moreno <4879373+juanpedromoreno@users.noreply.github.com>
|
Hey @cb372 I'm fairly late to this party... but this looks awesome, thank you very much! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #114
Added a new
identifiercombinator toPrinter, which turns arbitrary strings into valid Scala identifiers by wrapping them with backticks if necessary. Unfortunately it wasn't possible to actually use the combinator in that many places; I ended up directly calling the underlyingPrinter.toValidIdentifierinstead.I didn't want to write my own buggy implementation of the Scala lexical spec, so I used scalameta to take care of the logic to decide when something needs to be backticked.
The printing of Mu schemas/protocols was pretty trivial to fix. The OpenAPI printing, on the other hand, is much more complex. There is a lot more code being generated, and there was already a lot of string manipulation/normalization going on. Because of this, I'm not that confident I've covered every single case correctly, but the test coverage is at least high enough to convince me I haven't made the situation worse.
These changes have caused the OpenAPI codegen behaviour to change in a few cases. Now instead of names being "normalized" (which did not take account of the need to escape Scala reserved words), they are left as-is but wrapped in backticks as necessary when they are used as Scala identifiers. See e.g. this updated test for an example of the changes. There were also a couple of small unintended changes that were really hard to avoid without extensive refactoring, e.g. this change from
Statustostatus.