ARM: add `arm-resource-name-pattern` rule to allow disabling LintDiff `ResourceNamePattern` rule #359

tjprescott · 2024-03-04T21:51:54Z

Closes https://github.com/Azure/typespec-azure-pr/issues/3903

REST API Specs (6 violations)
Azure/azure-rest-api-specs#28085

OpenAPI Validator PR
Azure/azure-openapi-validator#669

azure-sdk · 2024-03-04T21:52:56Z

All changed packages have been documented.

azure-sdk · 2024-03-04T22:11:20Z

You can try these changes at https://cadlplayground.z22.web.core.windows.net/typespec-azure/prs/359/

Check the website changes at https://tspwebsitepr.z22.web.core.windows.net/typespec-azure/prs/359/

tjprescott · 2024-03-05T19:28:29Z

packages/typespec-azure-resource-manager/src/rules/arm-delete-response-codes.ts

-          const statusCodes = new Set([...httpOp.responses.map((r) => r.statusCodes.toString())]);
-          const expected = new Set(["204", "*"]);
-          expected.add(isAsync ? "202" : "200");
+      root: (program: Program) => {


This is just a refactoring of this existing rule to be more efficient. It doesn't change the behavior.

tjprescott · 2024-03-05T19:32:54Z

packages/typespec-azure-resource-manager/src/rules/arm-resource-name-pattern.ts

+            if (pattern === undefined) {
+              context.reportDiagnostic({
+                target: nameProperty,
+              });
+            }


So, per the LintDiff rule it says it's also fine to apply the default ARM pattern here if one is not provided. We have a couple options:

don't do that and just always throw this warning

always apply the default if not specified, in which case this linter rule actually becomes a warning diagnostic and the logic would change

provide an easy mechanism to provide the default pattern and have the error message tell people to use that (like we did for the error codes rule pointing to other templates).

@markcowl @timotheeguerin @johanste any thoughts on what is preferrable?

It would be preferable (to me) if there was an implicit default. But I'm not sure how many resources adhere to the same constraints.

That rule could also be an easy code fix candidate so user can auto fix

@timotheeguerin do we have documentation on how to do that?

https://typespec.io/docs/extending-typespec/linters#provide-a-codefix

For linter in particular

The auto suppress code fix in the compiler is a simple one that is similar in the logic

Also feels like adding a decorator on a node feels like a common code fix we should maybe build a common helper piece in the same way we have the update identfier codefix, don't need to do that in your pr but could use that as the base piece for it as a separate issue

Agreed, a codefix with the default value is the best way to document this. In general, I think we should make a codefix an acceptance criterion for new linting rules, where that is possible.

But, please, file an issue for adding the codefix for this, it is important we get this rule in.

Filed issue: microsoft/typespec#3044

I put a local codefix here that we can replace when the global helper is available.

.chronus/changes/arm-resourceNamePatternRule-2024-2-4-21-52-56.md

timotheeguerin · 2024-03-08T18:07:38Z

packages/typespec-azure-resource-manager/src/rules/arm-resource-name-pattern.ts

+            if (pattern === undefined) {
+              context.reportDiagnostic({
+                target: nameProperty,
+              });
+            }


That rule could also be an easy code fix candidate so user can auto fix

packages/typespec-azure-resource-manager/src/rules/arm-resource-name-pattern.ts

markcowl · 2024-03-22T00:48:44Z

packages/typespec-azure-resource-manager/src/rules/arm-resource-name-pattern.ts

+            if (pattern === undefined) {
+              context.reportDiagnostic({
+                target: nameProperty,
+              });
+            }


Agreed, a codefix with the default value is the best way to document this. In general, I think we should make a codefix an acceptance criterion for new linting rules, where that is possible.

markcowl · 2024-03-22T00:49:29Z

packages/typespec-azure-resource-manager/src/rules/arm-resource-name-pattern.ts

+            if (pattern === undefined) {
+              context.reportDiagnostic({
+                target: nameProperty,
+              });
+            }


But, please, file an issue for adding the codefix for this, it is important we get this rule in.

packages/typespec-azure-resource-manager/src/rules/arm-resource-name-pattern.ts

…e-name-pattern.ts Co-authored-by: Mark Cowlishaw <[email protected]>

markcowl · 2024-03-28T19:41:20Z

packages/typespec-azure-resource-manager/src/rules/arm-resource-name-pattern.ts

+        const resources = getArmResources(program);
+        for (const resource of resources) {
+          // find the name property
+          const nameProperty = resource.typespecType.properties.get("name");


This is likely to change soon, but is accurate for now

tjprescott force-pushed the arm/resourceNamePatternRule branch 2 times, most recently from 5769075 to 3e989be Compare March 4, 2024 21:55

tjprescott force-pushed the arm/resourceNamePatternRule branch 2 times, most recently from 1643029 to 65a18db Compare March 5, 2024 19:11

tjprescott marked this pull request as ready for review March 5, 2024 19:26

tjprescott requested review from bterlson, markcowl, allenjzhang and timotheeguerin as code owners March 5, 2024 19:26

tjprescott commented Mar 5, 2024

View reviewed changes

This was referenced Mar 5, 2024

Fix for arm-resource-name-pattern rule Azure/azure-rest-api-specs#28085

Closed

Disable LintDiff rules for TypeSpec 0.55 Azure/azure-openapi-validator#669

Merged

tjprescott force-pushed the arm/resourceNamePatternRule branch 3 times, most recently from 43af0a9 to 6e6f405 Compare March 8, 2024 18:03

timotheeguerin reviewed Mar 8, 2024

View reviewed changes

tjprescott force-pushed the arm/resourceNamePatternRule branch 2 times, most recently from 2c47c86 to 3c071ec Compare March 11, 2024 20:52

timotheeguerin reviewed Mar 12, 2024

View reviewed changes

packages/typespec-azure-resource-manager/src/rules/arm-resource-name-pattern.ts Outdated Show resolved Hide resolved

tjprescott force-pushed the arm/resourceNamePatternRule branch from 3c071ec to 05080c4 Compare March 12, 2024 19:48

markcowl requested changes Mar 22, 2024

View reviewed changes

tjprescott requested review from lmazuel, m-nash, iscai-msft, srnagar and joheredi as code owners March 22, 2024 19:45

tjprescott force-pushed the arm/resourceNamePatternRule branch from d5d8172 to ba13810 Compare March 22, 2024 20:15

timotheeguerin reviewed Mar 22, 2024

View reviewed changes

packages/typespec-azure-resource-manager/src/rules/arm-resource-name-pattern.ts Outdated Show resolved Hide resolved

tjprescott force-pushed the arm/resourceNamePatternRule branch from 39e45f6 to 9162849 Compare March 22, 2024 20:30

tjprescott commented Mar 22, 2024

View reviewed changes

packages/typespec-azure-resource-manager/src/rules/arm-resource-name-pattern.ts Outdated Show resolved Hide resolved

tjprescott force-pushed the arm/resourceNamePatternRule branch 2 times, most recently from 40b98a5 to f05e32f Compare March 28, 2024 17:48

tjprescott and others added 15 commits March 28, 2024 12:26

Closes Azure/typespec-azure-pr#3903

75421f8

Refactor delete-operation-response-code rule.

6d25b6a

Create arm-resourceNamePatternRule-2024-2-4-21-52-56.md

4a6e1a6

Make test cover child resources too.

47b19c8

Update .chronus/changes/arm-resourceNamePatternRule-2024-2-4-21-52-56.md

9132a75

Code review feedback.

09de75b

Closes Azure/typespec-azure-pr#3903

6fada28

Create arm-resourceNamePatternRule-2024-2-4-21-52-56.md

04dcd3b

Make test cover child resources too.

e02794c

Update .chronus/changes/arm-resourceNamePatternRule-2024-2-4-21-52-56.md

115465b

Update packages/typespec-azure-resource-manager/src/rules/arm-resourc…

e055230

…e-name-pattern.ts Co-authored-by: Mark Cowlishaw <[email protected]>

Add codefix.

c129227

Add codefix test.

9014857

Code review feedback.

e146569

Update default pattern.

a4a565c

tjprescott force-pushed the arm/resourceNamePatternRule branch from f05e32f to a4a565c Compare March 28, 2024 19:28

markcowl approved these changes Mar 28, 2024

View reviewed changes

tjprescott merged commit 8d9d99a into main Mar 28, 2024
16 checks passed

tjprescott deleted the arm/resourceNamePatternRule branch March 28, 2024 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARM: add `arm-resource-name-pattern` rule to allow disabling LintDiff `ResourceNamePattern` rule #359

ARM: add `arm-resource-name-pattern` rule to allow disabling LintDiff `ResourceNamePattern` rule #359

tjprescott commented Mar 4, 2024 •

edited

Loading

azure-sdk commented Mar 4, 2024 •

edited

Loading

azure-sdk commented Mar 4, 2024

tjprescott Mar 5, 2024

tjprescott Mar 5, 2024

johanste Mar 6, 2024

timotheeguerin Mar 8, 2024

tjprescott Mar 12, 2024

timotheeguerin Mar 12, 2024

timotheeguerin Mar 12, 2024

timotheeguerin Mar 13, 2024 •

edited

Loading

markcowl Mar 22, 2024

markcowl Mar 22, 2024

tjprescott Mar 22, 2024

timotheeguerin Mar 8, 2024

markcowl Mar 22, 2024

markcowl Mar 22, 2024

markcowl Mar 28, 2024

ARM: add arm-resource-name-pattern rule to allow disabling LintDiff ResourceNamePattern rule #359

ARM: add arm-resource-name-pattern rule to allow disabling LintDiff ResourceNamePattern rule #359

Conversation

tjprescott commented Mar 4, 2024 • edited Loading

azure-sdk commented Mar 4, 2024 • edited Loading

azure-sdk commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timotheeguerin Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ARM: add `arm-resource-name-pattern` rule to allow disabling LintDiff `ResourceNamePattern` rule #359

ARM: add `arm-resource-name-pattern` rule to allow disabling LintDiff `ResourceNamePattern` rule #359

tjprescott commented Mar 4, 2024 •

edited

Loading

azure-sdk commented Mar 4, 2024 •

edited

Loading

timotheeguerin Mar 13, 2024 •

edited

Loading