A few design questions keeping the changes to the certificates package in mind. The same applies to keys & secrets as they share the same design. I am guessing we used the .Net implementation as a reference here and since this is a preview, none of these questions block the release, but we should discuss these nevertheless so that we can make the right call for GA

• KeyVaultCertificatesIdentifierCollectionName
  • Do we really need this type? Can we not inline the string literals? Having the type is helpful if there are operations on it for which the user would need type assistance. Given that the goal here is only to say that there are a finite set of values, would inlining make more sense?
• collection property & validation for it in the parse function
  • I could not find any public api in the package that would take a collection. So, even after parsing the KeyVault Identifier, the user would not have any use for it other than perhaps logging. What is the benefit of having validation around this in the parse function?
• id property in the parsed output
  • From what I understood, this is the identifier i.e. the url passed into the parse function. What is the use of having it in the output as well? We don't mutate it and the value is the same as what the user provided right?
• parseKeyvaultIdentifier() in core/utils.ts file
  • Correct me if am wrong... my assumption was that the core folder corresponds to the auto generated code from the swagger. The utils.ts file seems handwritten. If this is indeed hand-written, then this should perhaps move to keyvault-common?

KeyVaultCertificatesIdentifierCollectionName Do we really need this type? Can we not inline the string literals? Having the type is helpful if there are operations on it for which the user would need type assistance. Given that the goal here is only to say that there are a finite set of values, would inlining make more sense?

Sure, I can do that.

collection property & validation for it in the parse function - I could not find any public api in the package that would take a collection. So, even after parsing the KeyVault Identifier, the user would not have any use for it other than perhaps logging. What is the benefit of having validation around this in the parse function?

Not sure, let's ask @heaths . Perhaps I got something wrong.

id property in the parsed output - From what I understood, this is the identifier i.e. the url passed into the parse function. What is the use of having it in the output as well? We don't mutate it and the value is the same as what the user provided right?

Not sure, let's ask @heaths . Perhaps I got something wrong.

parseKeyvaultIdentifier() in core/utils.ts file - Correct me if am wrong... my assumption was that the core folder corresponds to the auto generated code from the swagger. The utils.ts file seems handwritten. If this is indeed hand-written, then this should perhaps move to keyvault-common?

Sure! I had forgotten that this file was hand-written. A year has passed 😮

Re: collection

I agree that this doesn't really need to be public. Internally, .NET already has types that do this (in fact, I'll likely just "forward" most of the functionality to them) and we only use Collection to effectively assert that we are using, say, a secret in SecretClient. You're right it doesn't really have value to callers.

Re: id

The value I see is associating an instance of the KeyVault*Identifier to the value passed in, rather than keeping any sort of map yourself (as a caller). Ideally, it's merely a reference so has no significant memory impact. But the URIs themselves can be consumed elsewhere, like references in app configuration (in the general sense). I think there's value in keeping it as a "pass-through" property.

I agree that this doesn't really need to be public. Internally, .NET already has types that do this (in fact, I'll likely just "forward" most of the functionality to them) and we only use Collection to effectively assert that we are using, say, a secret in SecretClient. You're right it doesn't really have value to callers.

@sadasant, I would recommend that we remove the collection from the parsed output and remove any validations for it in the new exposed parser function if no one from the feature crew has any further concerns.

The value I see is associating an instance of the KeyVault*Identifier to the value passed in, rather than keeping any sort of map yourself (as a caller). Ideally, it's merely a reference so has no significant memory impact. But the URIs themselves can be consumed elsewhere, like references in app configuration (in the general sense). I think there's value in keeping it as a "pass-through" property.

In that case, I would recommend changing the name in the output for this id to make it clear that it was the input id. Otherwise, when I look at a type called Parsed.... I expect it to contain the parsed bits. A reference to the original should be named that it is easily at a glance differentiable from the others.

This name should be consistent across the languages, so am ok keeping it as id for now, but please log an issue so that we can resolve the naming concern before GA

While I think "ID" (regardless of casing) is important and consistent across usage within a language as well as across languages and even libraries, perhaps "OriginalId" would make more sense here?

While I think "ID" (regardless of casing) is important and consistent across usage within a language as well as across languages and even libraries, perhaps "OriginalId" would make more sense here?

Original ID feels weird, since we didn't change it... I would prefer just id, at least myself.

inputId, unparsedId, fullId, completeId: the world is full of choices

inputId, unparsedId, fullId, completeId: the world is full of choices

I'll add "SourceId" into the mix as well 😄

I like sourceId!

We could also use "identifier", how does that sound?

identifier would be no different than id in this case

We're currently arguing if we should continue to follow this approach or depart from it to some extent. Once we reach an agreement internally, I will update this PR.

Welp, I ended up merging this without all of the changes. My bad. A new PR with an update is coming.