[SOAR-0013] Idiomatic naming strategy

[czechboy0]

Motivation

See proposal

Modifications

N/A

Result

N/A

Test Plan

N/A

[simonjbeaumont]

Thanks @czechboy0! Review started: https://forums.swift.org/t/proposal-soar-0013-idiomatic-naming-strategy/76242.

[hactar]

I'm very happy to see that work is being done to improve the ease of use of the generator and like all changes described here. Personally I'd argue to make idiomatic the default and bump the version number to signify the "break", documenting that the old behavior is still available via a config change: if we're doing this to make it easier for new people to use this, then they shouldn't have to configure it.

But I hope this proposal can go a bit further. For me, it would not fix the first experience I had with this generator:

When I started using this generator: pretty much any config file that I was provided never contained operationId. This lead to even the simplest operations like /claims to be named _sol_claims. When I then started using Xcode's auto complete to help me write my code, I typed "try await client.c" and was expecting autocomplete to complete to claims. Instead it did nothing, and I thought the generator was broken. Only through digging and reading documentation did I find that it did actually work, but the call is try await client._sol_claims(...) - and a function that starts with an underscore usually triggers all warnings for me, cause I'm used to things starting with underscore to be private. I also had to look up what "sol" was, I only new "/" as "slash".

I hope the proposal can be amended to have an idiomatic approach for naming operations with slashes in them, as I'm pretty sure this will have snagged others as well when they started.

TLDR:

• make idiomatic the default
• cover paths containing slashes too

[simonjbeaumont]

@hactar thanks for taking the time to provide your feedback.

Personally I'd argue to make idiomatic the default and bump the version number to signify the "break", documenting that the old behavior is still available via a config change: if we're doing this to make it easier for new people to use this, then they shouldn't have to configure it.

If we were to release a new major version, that would be on the table, but we cannot do this in a SemVer minor release.

When I started using this generator: pretty much any config file that I was provided never contained operationId.

That's an interesting thought. This proposal currently doesn't do anything different to address operations without an operation ID.

I hope the proposal can be amended to have an idiomatic approach for naming operations with slashes in them, as I'm pretty sure this will have snagged others as well when they started.

OOI, what would you write by hand for an operation that did have an operationId but that ID contained a slash?

I'm wondering if it would be enough to just drop the leading slash, if present, either in the explicit or implicit operation ID.

[czechboy0]

Hi @hactar,

thanks so much for the thoughtful feedback.

I'll get the first one out of the way - while I agree that for most people this will be the new default, and that it would be nice to not have to explicitly configure it, we're not ready to go through the ecosystem breakage that a 2.0 would cause right now. Plus, we might accumulate more feature flags over the next 6-12 months, so I think it's more likely that we'd ship a 2.0 once there are multiple highly valuable improvements that need to be enabled explicitly. I don't believe this proposal alone meets that bar, but again, in broad strokes I agree with your reasons to ship a new major.

The second point you made, about slashes leading to suboptimal names, and those being very common when no operationId is specified - you're absolutely right, I didn't even think about this case. (That's why I'm glad we do these proposals 🙂)

Considering your case, and complicating it a bit to help us think up a solution, if no operationId is specified, and the method + path are something like:

GET /claims/{claimId}/notifications

what would you like the synthesized operationId to be?

// Edit: Si just beat me to it 🙂
// Edit 2: Just tested that the current operation name we get is get_sol_claims_sol__lcub_claimId_rcub__sol_notifications

[hactar]

Default setting: I understand - then I think we should at least ensure it is in the default example of the tutorials so anyone starting out will see it and use it if they copy/paste.

Operations:
get_sol_claims_sol__lcub_claimId_rcub__sol_notifications awesome, yes maybe I remember incorrectly about the _ being the first char then, but still this to me looked more like a debug string than anything else, especially not knowing what sol and lcub were (and the tutorials appeared to generate nice names, so I thought it was broken).

So I'm going to approach this from the other side, what would the perfect easy to read swift function be (assuming claimId is a String)? To me it would be

func getClaimNotifications(for claimId: String) async throws -> [Notification]

So what happened?

• the "sol" were dropped, because usually the slashes are more a "space" than anything else
• the "mid path" variable was removed from the function name because its actually a function parameter
• the "s" from claims was removed because it only makes sense for the root case (GET /claims) but not as soon as any id is present, there you're actually only getting a claim's notification typically in REST Apis

Now I do understand that my function is very idiomatic and makes assumptions on how things should be (I'm a SwiftUI guy, thats how we role 😄). If we want to be less idiomatic about it, some of the steps above could be removed:

• getClaimsNotifications would still be great
• getClaimsClaimIdNotifications is still readable
• getClaims_lcub_claimId_rcub_Notifications is beginning to look like a debug string and mixes camel case and string case , so getClaimsLcubClaimIdRcubNotifications would be a bit better, still not pretty though.

As soon as we start adding any sol or lcub or double underscore I'd argue that things become difficult to read. Swift is a camel case and not a snake case language, so I'd try to prevent underscore usage whenever possible.

Ss far as I understand the whole sol thing was done to prevent naming clashes - but could we make it so that it only triggers if really required - if an actual clash occurs?

[VyrCossont]

Question about name overrides in the config file: does that affect properties, types, or both, and is there a way to do it for only a specific property on a specific type? Use case would be fixing the occasional confusing name in the API I'm consuming.

[deirdresmtoo]

@hactar had some good ideas, but I wanted to address this one:

• the "s" from claims was removed because it only makes sense for the root case (GET /claims) but not as soon as any id is present, there you're actually only getting a claim's notification typically in REST Apis

Inflections for plurals are very irregular, and many singular forms are also plurals. Rails gave up on providing comprehensive auto inflections for this reason (I contributed an inflection patch in the last version where they were being accepted), and instead allowed people to specify mappings. This will just chew up people points, especially once you get to things like correct inflections for octopus or correct singular for hooves.

The folks who wrote the API specified claims as plural, and we should stick to that without a compelling reason to deviate.

[hactar]

Inflections for plurals are very irregular, and many singular forms are also plurals. Rails gave up on providing comprehensive auto inflections for this reason (I contributed an inflection patch in the last version where they were being accepted), and instead allowed people to specify mappings. This will just chew up people points, especially once you get to things like correct inflections for octopus or correct singular for hooves.

I agree with this. Inflections are difficult. I was describing my perfect generated function name for the provided example. A generic version would be making a lot of assumptions, like the API being in english. If we could do this close to perfectly in most cases it would be awesome, but its not as simple as "remove an 's' if its at the end of a word.

[deirdresmtoo]

@hactar One possibility might be allowing a struct (or YAML file) to help with inflections e.g.:

struct Inflection {
    let className: String   // Client
    let singular: String    // client
    let plural: String      // clients
}

When you get to languages with noun cases (e.g., many Eastern European languages), plural noun forms get more complex, but we're not generating plural forms, just trying to determine the singular from a given plural used in the openapi.yaml file, so this might be enough for most languages. It even offers a path for those that use pronouns to indicate number, e.g., Polynesian languages.

I think it would be cool if we could generate the function names with a singular form for singular items like your getClaimNotifications example, but it should default to plural if the inflection forms aren't specified (by whatever method is chosen).

[czechboy0]

Thanks again @hactar for bringing up the important topic of synthesized operationIds. I updated the proposal with a solution that's close to your generalized solution: by turning slashes into underscores, and dropping curly braces. In concrete terms, that means that an operation without an explicit operationId that looks like:

GET /claims/{claimId}/notifications

would get the method name: get_claims_claimId_notifications. We also considered dropping the underscores, which would result in getClaimsClaimIdNotifications, but felt that it loses the separation between path components, which is important for understanding the purpose of the path. It's still hopefully a big improvement over get_sol_claims_sol__lcub_claimId_rcub__sol_notifications. And we should always nudge adopters to add explicit operationIds to the OpenAPI doc, which offers the ultimate flexibility.

Question about name overrides in the config file: does that affect properties, types, or both, and is there a way to do it for only a specific property on a specific type? Use case would be fixing the occasional confusing name in the API I'm consuming.

It affects all strings from the OpenAPI document that the generator needs to turn into type/method/property names in the generated Swift code. So currently there isn't any way to only do it for one property in one type, but not for an identically named property in another type. You either use the original generated name, or you override it for all occurrences by using nameOverrides.

Also, thanks @hactar and @deirdresmtoo for the exploration of a more sophisticated automatic naming strategy, but as you correctly identified, that'd be much more complex due to the assumptions about the language and patterns used, and I'd consider that out-of-scope for now.

[hactar]

@czechboy0 Awesome, thanks! Yes, this will make things without operationIDs much useable.

I'd still argue that the resulting API should strive for camel case and not snake case - it is a Swift API guideline after all - it feels really weird to me that the Swift OpenAPI generator would generate API that does not comply to the guidelines: https://www.swift.org/documentation/api-design-guidelines/#conventions . The capitalization would still give away the separation between path components, so I don't see the added benefit of snake case?

But, even if you guys do decide to stick to snake case, this will proposal will be a great improvement!

[czechboy0]

The capitalization would still give away the separation between path components, so I don't see the added benefit of snake case?

I was thinking that if you have a path like GET /foo/{fooId}/bar, that get_foo_fooId_bar is less ambiguous than getFooFooIdBar, as from the latter, you could assume the path is really GET /fooFoo/idBar, for example. But maybe that's not as important to map back in.

What do you think, @simonjbeaumont? I'm actually fine with either now.