[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.

[simonjbeaumont]

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

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

IMO there's an important distinction here that I think has been missed: the goal of avoiding snake case is a good one but it should not be confused with avoiding underscores in all places.

There is no snake case in the proposal—because snake-case and camel-case are about word separation.

The goal is to generate the most idiomatic code we can, within reason, from a generalisable, opinionated strategy. That means we should aim to generate code that is most similar to what would be written by hand, even if that does have underscores.

The clearest example is how to handle the translation of version2.0. For this we have suggested version2_0 as it felt hard to argue that version20 would be acceptable. This is also what I would have written by hand—and, it turns out, it is what we wrote by hand, in the generator when dealing with different version constants for the OpenAPI spec. @hactar I'm curious if you have any different opinion on the underscore here, or alternative spelling you would use in hand-written code?

If we agree that we shouldn't remove the . from version2.0, then we've arrived at a question that needs generalising that has nothing to do with casing: which symbols should we simply remove, and which should we replace, and with what.

If the feedback is that / should be removed, I'm OK with us taking that stance. But IMO it should be on the grounds that we've thought about it distinct from casing, and that we're not striving for a zero underscore policy.

[hactar]

I agree with you that we need to be careful not to mix up different aspects of the generator.

In the case of an operation, the generator is taking the http method, a word, and the path components, also usually words - frequently even forming sentences if read together. For the example of GET /map/styles, this can be read as a sentence: "get map styles". These words now need to be joined in a way that makes them readable to a swift developer, because '/' and ' ' is not valid within a function name. So some options would be

• join them without modification and without a separator, then we get: "GETmapstyles()". Not really good.
• normalize case and use "_" for joining: "get_map_styles()". This is snake case.
• adjust case and use no separator: "getMapStyles()". This is camel case.

As I see it the goal of the Swift OpenAPI Generator is to take an OpenAPI spec and turn it into an API that feels like a native Swift API. Swift APIs are camel case, so I'd pick the last option here.

Now "version2.0" is a different pair of shoes. I'm assuming you're referring to something like GET /map/styles/version2.0. Here version2.0 is a path component. The . is a separator within the component, not a path component separator. So yes, here we need to find something to replace the "." with, and that can be a "_" without turning the API itself into snake case. The generated function name would be "getMapStylesVersion2_0()", which is still very readable, and still camel case - even though there is an underscore in it.

[simonjbeaumont]

Thanks @hactar for continuing to reason this one through with us.

For the example of GET /map/styles, this can be read as a sentence: "get map styles".

If one views the slash separators in a path as equivalent to word separators, then it follows that substituting them with underscores results in snake case. My position was/is that the slashes held more semantic meaning than just word separation, so the use of underscores in their place felt as natural to me as in the version2.0 to version2_0.

However, I think for those that do see them as word separation, the use of underscores is quite an eye sore. So, I'm happy in this case to remove them and treat them like word separators as I think, averaging across the pool of users, this will cause the least surprise.

What's your thinking on this one now @czechboy0?

[czechboy0]

Yeah, I'm happy to change the proposal to reflect that.

[deirdresmtoo]

I agree with what @hactar and @simonjbeaumont have said about their slash and underscore comments.

In particular, Si's comment about slashes holding more semantic meaning. An API that has GET /map/styles implies there might be more under map, e.g., GET /map/settings, where the slash is serving a function much like an aisle in a library, grouping related topics.

Where an api that had GET /mapstyles implies that's all there is there.

Path component cleanup via _ seems sensible (e.g., the Version2.0 -> version2_0 example).

[czechboy0]

I think in the most recent comments, we seem to have landed on just dropping the slashes rather than replacing them with underscores. Is that still okay with you, @deirdresmtoo?

[deirdresmtoo]

Absolutely, apologies for leaving that unclear.

[czechboy0]

Ok folks, here's the diff of the latest proposal changes (version 1.2): e1f5e42

[czechboy0]

Pushed a revision of the proposal: v1.3: 73fd986

These changes are the result of needing to further clarify a few points that came up during implementation.

[czechboy0]

@simonjbeaumont Updated the status as implemented in 1.6.0 (the next minor to go out, since the code is now merged).

I'm preparing a PR for the examples/integration test.

[czechboy0]

But this PR is ready to get reviewed/merged.