[v6] Naming fixes #625
[v6] Naming fixes #625
Conversation
joheredi
commented
Apr 23, 2020
joheredi
commented
- Add more reserved words
- Provide more context info to normalizeName to get better prefixes when find a reserved name
- Make sure to guard for reserved names when normalizing in Transforms
- Use getOperationFullname function to avoid inconsistencies
- Update generated files and tests
| */ | ||
| export interface HeaderCustomNamedRequestIdParamGroupingHeaders { | ||
| export interface HeaderCustomnamedrequestidparamgroupingHeaders { |
There was a problem hiding this comment.
Looks like these are getting lowercased unexpectedly?
test/integration/generated/azureSpecialProperties/src/models/mappers.ts
Outdated
Show resolved
Hide resolved
test/integration/generated/azureSpecialProperties/src/operations/xMsClientRequestId.ts
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,434 @@ | |||
| /* | |||
| * Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
Looks like new files are being added for the renamed operation groups, you might need to delete the leftover files with the old name
src/utils/nameUtils.ts
Outdated
| @@ -4,7 +4,70 @@ import { Operation, OperationGroup } from "@azure-tools/codemodel"; | |||
| import { getLanguageMetadata } from "./languageHelpers"; | |||
| import { TypeDetails, PropertyKind } from "../models/modelDetails"; | |||
|
|
|||
| const ReservedModelNames = ["Error", "Date"]; | |||
| const ReservedModelNames = [ | |||
There was a problem hiding this comment.
Should we be using one set of rules for all models?
I can see one set of rules for things that can be 'new'd up. For example, OperationGroups can be instantiated directly by the user, so you need to guard against names like Date, String, etc.
However, properties can be pretty much anything. You can have an interface that looks like this:
interface Foo {
string: string;
}and that works just fine. Likewise for parameters:
function(string: string) {}However, in the parameters case you definitely don't want to allow any reserved words (and that's probably true for properties too so that params/properties stay consistent across operation inputs and outputs.
Side-note: Can you make this list ordered alphabetically? It'll be easier to add things or check what's in it then.
There was a problem hiding this comment.
Alright, thanks @daviwil and @chradek for your feedback around this. I agree with you and here's what I'm thinking:
-
Have metadata on the reserved words to indicate in which cases we want special handling. So that we just maintain a single list of reserved words but can choose case by case if we need to suffix the name.
-
File, operations and properties, don't need suffixing as @chradek mentioned above, so I have skipped a suffix in those cases.
-
OperationGroups, Parameters, Classes and Interfaces(for consistency) would need to be guarded against a subset of reserved words, such as Error, String, Date, Boolean, etc
- OperationGroups will get Operations suffix.
- Parameters would get Param suffix
- Classes and Interfaces will get Model suffix
|
|
||
| const Newable = [NameType.Class, NameType.Interface, NameType.OperationGroup]; | ||
|
|
||
| const ReservedModelNames: ReservedName[] = [ |
There was a problem hiding this comment.
I like this! Seems like all of them need NameType.Parameter so it feels like that should be checked by default in guardReservedNames, but I suppose it makes things less clear to not have it explicitly in the list.
There was a problem hiding this comment.
Yeah, I thought about that. However by having it there we don't need any NameType specific checks in guardReservedNames and as you said makes it makes it clear and gives us flexibility in case we decide we don't want to suffix a specific reserved word. For instance string would be a valid parameter name
