[SOAR-0001] Improved mapping of identifiers

[denil-ct]

Motivation

Encoding would help distinguish types properly. For eg. both a b and a_b would be rendered as a_b. This change would make it render a20b and a_b respectively.

Modifications

Added a hexadecimal encoding for un-supported characters rather than having just an underscore for everything.
Removed redundant keywords.

Result

Proper differentiation between names as explained in the motivation, along with that, it also reduces the reserved keywords set by removing symbols and any keywords that prepend such symbols.

Test Plan

Tested and verified for several combinations of special characters in various positions in-between alphanumeric characters.

[czechboy0]

Hey @denil-ct, thanks for breaking this out.

I'm on the fence about this.

On the plus side, it helps disambiguate special symbols, so it even helps with #21.

On the other hand, the hex code can seem a little arbitrary.

To help us see what these will look like in practice, can you add a bunch of examples to this unit test? Like 10 different ones with various special symbols in different parts of the name, to help us see the impact. https://github.com/apple/swift-openapi-generator/blob/main/Tests/OpenAPIGeneratorCoreTests/Extensions/Test_String.swift#L19

Thanks 🙏

cc @simonjbeaumont for thoughts, I'm genuinely split here.

[denil-ct]

Sure, will add some. I went with hex because I was influenced by the comment stating similar to url encoding. If the numbers seem a bit arbitrary we can explore some other form as well.

[czechboy0]

Yeah, it's a fair starting point. @simonjbeaumont recently added a special case for empty strings (_empty), and we could even have a hard-coded map for common symbols, e.g. + -> plus, # -> pound, etc, so that the hex would only happen for rarely used symbols.

One thing to keep in mind - this is an API-breaking change, so will have to be in a minor bump, as if people update the generator, they'd get different symbol names. That's okay, just making it explicit here.

[simonjbeaumont]

@denil-ct thanks for thinking about this one.

One thing I do like about this approach is it generalises. For replacements within a string we might also benefit from some delimiting to make it clearer it's the result of mangling.

Yeah, it's a fair starting point. @simonjbeaumont recently added a special case for empty strings (_empty), and we could even have a hard-coded map for common symbols, e.g. + -> plus, # -> pound, etc, so that the hex would only happen for rarely used symbols.

We could find a middle ground. I think that the empty string probably deserves its own special case because there's nothing else to meaningfully identify it—i.e. having just _20 is pretty uninformative.

It makes me wonder if the heuristic might be to only make a "wordy" replacement when there's nothing else other than the symbol?

This one is definitely a tradeoff and anything we do will have some drawback.

Another compromise here could be to generate the hex-encoded thing always and a type alias and or function wrapper with a nice name for a few cases, e.g. +1 would generate _2b1 and an alias _plus1. We can then have something that's uniform with some opt-in syntactic sugar?

[czechboy0]

Mostly on the same page with you both, but since this will have a pretty visible impact on the naming logic, could you do a proposal (see our docs for the template)? Doesn't have to be long, just describe the overall naming logic:

• use as-is if all characters are legal for a Swift identifier, and doesn't start with a number
• prefixed with an underscore when starts with a number
• if it's a known keyword, add an underscore
• each character is checked, if it's not safe in a Swift identifier, then it's looked up in a hard-coded map (list the hard-coded characters and their word replacements), and if it's not in the map, is converted to its hex code

The above with a bunch of diverse examples of inputs and outputs.

Doing the typealias would be a bit tricky due to namespacing and uniqueness, so I'd prefer if there was exactly one Swift-safe term for an input string.

Again, this will be a breaking change, so we want to make sure we understand the full implications.

[denil-ct]

Sure, that sounds good. Will create one and we'll proceed from there.

[czechboy0]

Can you put the PR into a draft state (or close it) until we have a proposal? Just to make clear that you're not waiting on us to take any action, like a review - thanks. 🙂

[denil-ct]

Ah sure, no issues.

[denil-ct]

Hey @czechboy0 @simonjbeaumont

I have updated the PR description with the proposal. Do let me know if anything is missing, and what the next steps are. Also should I pull this out of draft state?

[czechboy0]

Thanks for writing this up, @denil-ct! Could you add the proposal as a markdown file to the Proposals directory, next to the template? That'll allow us to comment on specific lines, thanks! You can then remove it from the PR description, to avoid having to update two places as we iterate.

[denil-ct]

Have added the proposed changes, along with updated examples. Looks too wordy in some cases imo.

[czechboy0]

Great! One more thing, we just recently updated the proposal process a tiny bit, see #91

So can you split the proposal into a separate PR from the code change, please? (Thanks for helping us figure out this overall process by test-driving it 🙂)

[simonjbeaumont]

Great! One more thing, we just recently updated the proposal process a tiny bit, see #91

So can you split the proposal into a separate PR from the code change, please? (Thanks for helping us figure out this overall process by test-driving it 🙂)

The docs you have here (about how we handle special characters) are pretty useful to folks generally beyond those reviewing the code change. @czechboy0 maybe the proposal should be replaced by an addition to the DocC?

[denil-ct]

@czechboy0 @simonjbeaumont

Should I proceed with splitting up the proposal and implementation?

[simonjbeaumont]

@denil-ct First off, thank you for jumping in—on this and other issues!

We'll want to proceed with a bit more caution with this change compared to the other contributions, as the choices we make here—and any subsequent changes to the area—will have an impact on the generated API.

This might mean waiting for us to have tests around the API changes in generated code, so I just wanted to manage expectations on your side.

Should I proceed with splitting up the proposal and implementation?

Yes, let's first land this as a proposal and let others chime in with any feedback before we land the implementation.

When we do land the implementation, we'll want to have some good testing around this too.

[denil-ct]

I totally understand, and there is no reason to rush, as the current solution is decent enough in many cases. Let's follow the age old measure twice, cut once approach for this.

[denil-ct]

Marking as draft until the proposal is approved.

[denil-ct]

@czechboy0 please add a blocked tag here as well

[czechboy0]

Done ✅

[czechboy0]

@denil-ct Ok, you're unblocked now, please provide the new implementation in this method, and add a bunch of unit tests for it please.

[czechboy0]

Also, changing the label from semver/minor to semver/patch, because this will be hidden behind a feature flag in 0.1.x. Only once we enable the feature flag unconditionally in 0.2.x will it be a breaking change, so we'll mark semver/minor that PR, not this one.

[czechboy0]

Awesome, thank you!

Just a reminder, it's currently disabled behind the proposal0001 feature flag, but once it lands, anyone can test it out by enabling the feature flag (in CLI or in their config file).

The feature will be enabled unconditionally in the next breaking version, i.e. 0.2.0.