Validation rule to check if operationId nouns conflict with model names #2150
Conversation
|
|
||
| namespace AutoRest.Swagger.Validation | ||
| { | ||
| public class OperationIdNounPluralizedFormValidation : TypedRule<string> |
There was a problem hiding this comment.
I'm not sure about the naming of this class, should be more like "OperationIdNounConflictWithModel" ? What the rule is trying to enforce is not about pluralizing the noun, it's about not conflicting with another name being used for another class name (class name of a model vs. class name of the one containing the operations).
| /// <summary> | ||
| /// The severity of this message (ie, debug/info/warning/error/fatal, etc) | ||
| /// </summary> | ||
| public override Category Severity => Category.Error; |
There was a problem hiding this comment.
Let's check on this being an Error or not, as I was looking into this rule, I see that AutoRest will disambiguate on its own, calling the class that contains the operations "Operations", and keeping the "" as the name of the class of the model. Since this doesn't cause a code generation problem, but more of a "better name" for customers, it should be a warning in my opinion. Thoughts? @salameer @kirthik @amarzavery
There was a problem hiding this comment.
Agree with @veronicagg. Of course the decision makers might have thought of this and just love our customers very much: Indeed those suffixes are annoying - not only because they are long, but they also sometimes appear/disappear during tiny Swagger revisions (conflicting model definitions was introduced, existing model definition was renamed, etc...), which unnecessarily breaks existing code.
There was a problem hiding this comment.
@dsgouda maybe my comment wasn't clear enough, I would turn this into a warning at the moment, since it doesn't break code generation. You could also add in the message, if the name is not update, generated code will add a string like "Operations". to disambiguate.
| /// <remarks> | ||
| /// This may contain placeholders '{0}' for parameterized messages. | ||
| /// </remarks> | ||
| public override string MessageTemplate => Resources.OperationIdNounPluralizedMessage; |
There was a problem hiding this comment.
if you update the name of the class, please update this message name also.
| public override ValidationCategory ValidationCategory => ValidationCategory.SDKViolation; | ||
|
|
||
| /// <summary> | ||
| /// This rule passes if the operation id doesn't contain a repeated value before and after the underscore |
There was a problem hiding this comment.
this doesn't look like the right comment.
| @@ -349,4 +349,7 @@ Modelers: | |||
| <data name="ParametersPropertiesValidation" xml:space="preserve"> | |||
| <value>Parameter Must have the "name" property defined with non-empty string as its value</value> | |||
| </data> | |||
| <data name="OperationIdNounConflictingModelNamesMessage" xml:space="preserve"> | |||
| <value>OperationId has a noun that conflicts with one of the model names in definitions section. Consider using the pluralized form for '{0}'</value> | |||
There was a problem hiding this comment.
should it be "Consider using the plural form of..." instead of "pluralized ? :)
| @@ -0,0 +1,57 @@ | |||
| { | |||
| "swagger": "2.0", | |||
There was a problem hiding this comment.
you may want to update the name of the .json file.
| @@ -592,6 +592,13 @@ public void EmptyParameterNameValidation() | |||
| var messages = ValidateSwagger(Path.Combine(Core.Utilities.Extensions.CodeBaseDirectory, "Resource", "Swagger", "Validation", "empty-parameter-name.json")); | |||
| messages.AssertOnlyValidationMessage(typeof(ParameterNameValidation), 2); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void OperationIdNounPluralizedValidationTest() | |||
There was a problem hiding this comment.
could you update the name of the method here?
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
|
||
| using System; |
There was a problem hiding this comment.
could you update the name of the .cs file?
| /// <summary> | ||
| /// The severity of this message (ie, debug/info/warning/error/fatal, etc) | ||
| /// </summary> | ||
| public override Category Severity => Category.Error; |
There was a problem hiding this comment.
@dsgouda maybe my comment wasn't clear enough, I would turn this into a warning at the moment, since it doesn't break code generation. You could also add in the message, if the name is not update, generated code will add a string like "Operations". to disambiguate.
Addressing issue #1950