How to name certain enum values

[jhendrixMSFT]

Example names of enum values we get from TCGC.

V7.6_preview.1
V2022_12_01_preview

Propose we name them as follows.

V7Dot6_Preview
V2022_12_01Preview

The . character isn't legal in a name (syntax error), so must be removed.

For patterns like 01_preview the linter complains that it should be 01Preview.

[heaths]

I'd prefer _ everywhere e.g., V7_6_Preview_1 or V2002_12_01_Preview. The "Dot" makes it harder to read and just looks funny, and we shouldn't sandwich "01" and "Preview" together like that - especially when they weren't before. That's also consistent with idiomatic rust naming for both members and constants.

[jhendrixMSFT]

Agreed on _ instead of Dot.

Note that the linter will complain about 01_Preview which is why I suggested the sandwich.

[heaths]

Have an example of the linter message handy? We can always override it in those cases.

[jhendrixMSFT]

pub enum Versions {
    V2002_12_01_Preview,
}

warning: variant `V2002_12_01_Preview` should have an upper camel case name
  --> sdk\keyvault_secrets\src\generated\enums.rs:33:5
   |
33 |     V2002_12_01_Preview,
   |     ^^^^^^^^^^^^^^^^^^^ help: convert the identifier to upper camel case: `V2002_12_01Preview`
   |
   = note: `#[warn(non_camel_case_types)]` on by default

If we're happy with the name as-is then yeah we can suppress.

[heaths]

Cringe: "upper camel case". It's called PascalCase!

Yeah, I prefer the look of the underscores, and it jives with some of our other languages as well. I think just slap the #[allow(non_camel_case_types)] on the enum (or each member, but less noisy on the parent).

[heaths]

Actually, why are we generating an enum for versions at all? Per guideline discussions (and written guidelines), we always target the latest version but do allow the customers to pass in an api_version as a string. Isn't this similar to Go? So why should we define an enum? Just informational purposes or more as a backing store for supported versions?

[heaths]

I added guidelines, but we should answer my question above before merging. /cc @ronniegeraghty

[JeffreyRichter]

What if we just don't emit version enums at all?
We don't use these in Go or C++, for example.
We need to set the api-version corresponding to the SDK we are code generating but I suggest we just let customers pass in a string for any other api-version they want to try (but we do not officially support).
Also, while TypeSpec files will have multiple versions in them, we will remove preview versions from TypeSpec files as new previews/GAs ship and we will (probably) also remove old GA version if/when a new breaking GA version is introduced - a new breaking GA is tantamount to a new V 1.0. We're still discussing this last proposal.

[jhendrixMSFT]

The enums for the applicable API versions are defined by the tsp. Here's an example from KV.

https://github.com/Azure/azure-rest-api-specs/blob/56e1117535d28d9f0489c93d92a1ce01c3eeac2f/specification/keyvault/data-plane/Security.KeyVault.Secrets/main.tsp#L41-L53

What I'm reading is that we want to skip emitting these and just emit a String param instead, is that correct? Is it mandatory, or do we make it optional, with a client-side default value based on what the tsp tells us?

[JeffreyRichter]

Correct. Ignore the enum in the TypeSpec and use somehow set the default api-version value for clients to the version that is being used to generate the code. There must be some way to tell the code generator what api-version to generate code for, right?

[jhendrixMSFT]

Yes, we know the default value to use. I will make this change in the codegen today.

[heaths]

Per the guidelines - in which I figured we wouldn't have generated versions, mimicking Go and C++ - set the default in the Default impl:

pub struct ExampleClientOptions {
    api_version: String,
    client_options: azure_core::ClientOptions,
}

impl Default for ExampleClientOptions {
    fn default() -> Self {
        api_version: String::from("7.5"),
        client_options: Default::default(),
    }
}