[tcgc] add linter rulesets framework #1208

iscai-msft · 2024-07-19T21:18:40Z

fixes #1147

docs/libraries/typespec-client-generator-core/rules/require-client-suffix.md

…ypespec_azure_rulsets

azure-sdk · 2024-07-19T21:20:39Z

All changed packages have been documented.

✅ @azure-tools/typespec-azure-rulesets
✅ @azure-tools/typespec-client-generator-core

Show changes

`@azure-tools/typespec-client-generator-core` - feature ✏️

add linter rulesets to TCGC, for both generic and language-specific linter rules

`@azure-tools/typespec-azure-rulesets` - feature ✏️

add some tcgc rules to the list

timotheeguerin · 2024-07-19T21:22:51Z

packages/typespec-client-generator-core/src/lib.ts

-      },
-    },
-    "incorrect-client-format": {
+    "invalid-client-format": {


Is this actually used?

it's used when we're checking the input of @clientFormat. Is this valid?

I don't see it used in main anymore, could it have been removed

packages/typespec-client-generator-core/src/decorators.ts

packages/typespec-client-generator-core/test/rules/require-client-suffix.test.ts

docs/libraries/typespec-client-generator-core/rules/require-client-suffix.md

…ypespec_azure_rulsets

azure-sdk · 2024-07-22T14:42:43Z

You can try these changes here

🛝 Playground	🌐 Website	📚 Next docs

docs/libraries/typespec-client-generator-core/reference/linter.md

m-nash · 2024-07-22T17:50:30Z

docs/libraries/typespec-client-generator-core/rules/CONTRIBUTING.md

+1. Write the rule in `typespec-azure/packages/typespec-client-generator-core/src/rules/[rule-name].rule.ts
+2. Add reference to the rule in the `rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter]
+   - This will automatically add it to a ruleset called `:all` for `@azure-tools/typespec-client-generator-core`
+3. Add the rule to the enable list for [`data-plane.ts`][data-plane-ruleset] and/or [`resource-manager.ts`][resource-manager-ruleset] in the [rulesets][rulesets] package. You can set `enable` to `false` here, if you want to delay enabling


Do we really want to have different rules for data plane and mgmt plane? I know we are less strict with mgmt plane, but aren't the recommendations the same for both?

That's the current setup of azure-core-rulesets, where you explicitly list rules for the data plane and mgmt rulesets

there is some rules that management plane specific and some that shouldn't apply to mamangement plane. But the majority should apply to both

Do we have a single place to add a rule that applies to both or do we need to add the rule explicitly to both lists?

m-nash · 2024-07-22T17:51:43Z

docs/libraries/typespec-client-generator-core/rules/CONTRIBUTING.md

+
+**If you are adding a language-specific rule**, you will also need this extra step 4. Add reference to the rule in the `[language]Rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter]
+
+For Azure generations then, all rules, including all language-specific rules, will be run on the specs.


I think we want to limit this to the Tier 1 languages not necessarily all languages? Should we specify this? I assume Go could add their own rules in here and those wouldn't apply unless generating Go even for azure?

That's fair, let me update this to say that language-agnostic rules, and tier-1 languages will be run

m-nash · 2024-07-22T17:53:03Z

docs/libraries/typespec-client-generator-core/rules/CONTRIBUTING.md

+**If you are adding a language-specific rule**, you will also need this extra step 4. Add reference to the rule in the `[language]Rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter]
+
+For Azure generations then, all rules, including all language-specific rules, will be run on the specs.
+For unbranded generations, since we've added the rules into specific `best-practices:[language]` rulesets, you can explicitly specify a subset of rules in your `tsp-config.yaml`, i.e. if I only want Python best-practices, I could add this in my `tsp-config.yaml`:


This might create a bad situation where someone can turn off csharp rules but in reality they cannot. I think we might want to consider what flexibility a user would have over tcgc rules when running in azure context.

every azure service is enforced to have one of the 2 rules set enabled by the spec repo pipeline.

With rulesets, we have to give users the ability to turn them off because they are ultimately warnings. After talking with @timotheeguerin, you can't have error linter rules, only warnings. If there is a case where you explicitly want to error in csharp for a csharp-specific issue, the csharp emitter itself can throw a fatal error with reportDiagnostic, but the fatal error should have an analog in the linter warnings in tcgc that says "if you do this in csharp and ignore me, the csharp emitter will fail"

m-nash · 2024-07-22T17:54:03Z

docs/libraries/typespec-client-generator-core/reference/linter.md

+Available ruleSets:
+
+- `@azure-tools/typespec-client-generator-core/all`
+- `@azure-tools/typespec-client-generator-core/best-practices:csharp`


What does best practice mean here? Does it mean this is optional? If so is there another category which is not optional?

best practices is what I've called the ruleset for now, open to suggestions! I think the advice from the typespec team has been to use the word "best-practices", @timotheeguerin is this correct?

Aren't these rulesets optional and can be opted out?

oh I don't think we have a strong opinion on that name, we just have a placeholder @typespec/best-practices package that would contain misc linter to write good typespec.

I would say name this however makes the most sense. But I would note that I don't think this really should spend too much time on this as for all of azure they MUST use either the data-plane or resource-manager ruleset in teh spec repo. And I assume all those rules for all those language will be enabled.

I expect those more targeted ruleset might be more useful when we have the non azure version of tcgc(e.g. @typespec/client) where external customer might decide which language they care about.

m-nash · 2024-07-22T17:54:57Z

docs/libraries/typespec-client-generator-core/reference/linter.md

+
+Available ruleSets:
+
+- `@azure-tools/typespec-client-generator-core/all`


I am not sure all would ever be useful when considering non tier 1 languages as well as required rules vs best practices? Should we be a bit more granular on what all means?

all isn't a specificly-defined ruleset, it's the standard name for all of the rules existing in a package

docs/libraries/typespec-client-generator-core/rules/property-name-conflict.md

srnagar · 2024-07-23T20:21:58Z

docs/libraries/typespec-client-generator-core/reference/linter.md

+Available ruleSets:
+
+- `@azure-tools/typespec-client-generator-core/all`
+- `@azure-tools/typespec-client-generator-core/best-practices:csharp`


Aren't these rulesets optional and can be opted out?

srnagar · 2024-07-23T20:23:56Z

docs/libraries/typespec-client-generator-core/reference/linter.md

+| Name                                                                 | Description                                                             |
+| -------------------------------------------------------------------- | ----------------------------------------------------------------------- |
+| `@azure-tools/typespec-client-generator-core/require-client-suffix`  | Client names should end with 'Client'.                                  |
+| `@azure-tools/typespec-client-generator-core/property-name-conflict` | Avoid naming conflicts between a property and a model of the same name. |


Is this a required rule or is it a recommendation to avoid property and model having the same name.

all rules are in a sense optional its up to the user to enable them(directly or via a ruleset). For azure specs we require that the data-plane or resource-manager ruleset is used.

docs/libraries/typespec-client-generator-core/rules/CONTRIBUTING.md

srnagar · 2024-07-23T20:27:45Z

docs/libraries/typespec-client-generator-core/rules/CONTRIBUTING.md

+1. Write the rule in `typespec-azure/packages/typespec-client-generator-core/src/rules/[rule-name].rule.ts
+2. Add reference to the rule in the `rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter]
+   - This will automatically add it to a ruleset called `:all` for `@azure-tools/typespec-client-generator-core`
+3. Add the rule to the enable list for [`data-plane.ts`][data-plane-ruleset] and/or [`resource-manager.ts`][resource-manager-ruleset] in the [rulesets][rulesets] package. You can set `enable` to `false` here, if you want to delay enabling


Do we have a single place to add a rule that applies to both or do we need to add the rule explicitly to both lists?

docs/libraries/typespec-client-generator-core/rules/property-name-conflict.md

docs/libraries/typespec-client-generator-core/rules/CONTRIBUTING.md

srnagar · 2024-07-23T20:43:03Z

docs/libraries/typespec-client-generator-core/rules/CONTRIBUTING.md

+1. Write the rule in `typespec-azure/packages/typespec-client-generator-core/src/rules/[rule-name].rule.ts
+2. Add reference to the rule in the `rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter]
+   - This will automatically add it to a ruleset called `:all` for `@azure-tools/typespec-client-generator-core`
+3. Add the rule to the enable list for [`data-plane.ts`][data-plane-ruleset] and/or [`resource-manager.ts`][resource-manager-ruleset] in the [rulesets][rulesets] package. You can set `enable` to `false` here, if you want to delay enabling


I don't see step 3 done in this PR. Will this be added in a follow-up PR?

…ypespec_azure_rulsets

timotheeguerin · 2024-10-17T21:34:06Z

packages/typespec-client-generator-core/test/rules/require-client-suffix.test.ts

+import { requireClientSuffixRule } from "../../src/rules/require-client-suffix.rule.js";
+import { createSdkTestRunner } from "../test-host.js";
+
+describe("typespec-client-generator-core: require-client-suffix", () => {


nit: no need to add those top level describe with vitest, they just add extra nesting in the UI

packages/typespec-client-generator-core/src/decorators.ts

timotheeguerin · 2024-10-17T21:37:12Z

packages/typespec-client-generator-core/test/rules/property-name-conflict.test.ts

+});
+
+// TODO: reenable when rule is moved to tcgc and can resolve if clientName was set
+it.skip(`is valid if conflict resolved through @clientName("newName", "csharp")`, async () => {


i think those todo can be actioned on now?

should you be removing that rule from azure core too now?

iscai-msft added 2 commits July 19, 2024 16:23

add require-client-suffix

e8b5ac6

remove unused decorators

69bd8f1

iscai-msft requested review from lmazuel, m-nash, srnagar, joheredi, bterlson, markcowl, allenjzhang and timotheeguerin as code owners July 19, 2024 21:18

timotheeguerin reviewed Jul 19, 2024

View reviewed changes

docs/libraries/typespec-client-generator-core/rules/require-client-suffix.md Outdated Show resolved Hide resolved

Merge branch 'main' of https://github.com/Azure/typespec-azure into t…

befc3c8

…ypespec_azure_rulsets

timotheeguerin reviewed Jul 19, 2024

View reviewed changes

packages/typespec-client-generator-core/src/decorators.ts Outdated Show resolved Hide resolved

timotheeguerin reviewed Jul 19, 2024

View reviewed changes

packages/typespec-client-generator-core/test/rules/require-client-suffix.test.ts Outdated Show resolved Hide resolved

timotheeguerin reviewed Jul 19, 2024

View reviewed changes

docs/libraries/typespec-client-generator-core/rules/require-client-suffix.md Show resolved Hide resolved

iscai-msft added 7 commits July 19, 2024 18:31

add csharp-specific property-name-conflict

667dd62

add test to best-practices:csharp

e35c3e4

add docs and url to docs

b5cce28

add contributing doc

57d2f48

add changeset

1684417

Merge branch 'main' of https://github.com/Azure/typespec-azure into t…

01dd7e4

…ypespec_azure_rulsets

address comments

2de1f09

m-nash reviewed Jul 22, 2024

View reviewed changes

srnagar reviewed Jul 23, 2024

View reviewed changes

timotheeguerin mentioned this pull request Aug 12, 2024

[azure core] "@azure-tools/typespec-azure-core/property-name-conflict" throwing when renamed with @clientName #417

Open

iscai-msft added 2 commits October 17, 2024 16:04

address initial comments

a46581e

Merge branch 'main' of https://github.com/Azure/typespec-azure into t…

fc765ce

…ypespec_azure_rulsets

iscai-msft requested review from timotheeguerin, m-nash and srnagar October 17, 2024 20:22

iscai-msft added 2 commits October 17, 2024 16:54

format and lint

a83e2c2

add changeset

23e0e77

timotheeguerin reviewed Oct 17, 2024

View reviewed changes

packages/typespec-client-generator-core/src/decorators.ts Outdated Show resolved Hide resolved

timotheeguerin reviewed Oct 17, 2024

View reviewed changes

remove merge conflicts

ae9e97b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tcgc] add linter rulesets framework #1208

[tcgc] add linter rulesets framework #1208

iscai-msft commented Jul 19, 2024 •

edited

Loading

azure-sdk commented Jul 19, 2024 •

edited

Loading

`@azure-tools/typespec-client-generator-core` - feature ✏️

`@azure-tools/typespec-azure-rulesets` - feature ✏️

timotheeguerin Jul 19, 2024

iscai-msft Jul 22, 2024

timotheeguerin Oct 17, 2024

azure-sdk commented Jul 22, 2024

m-nash Jul 22, 2024

iscai-msft Jul 22, 2024

timotheeguerin Jul 22, 2024

srnagar Jul 23, 2024

m-nash Jul 22, 2024

iscai-msft Jul 22, 2024

m-nash Jul 22, 2024

timotheeguerin Jul 22, 2024

iscai-msft Jul 22, 2024

m-nash Jul 22, 2024

iscai-msft Jul 22, 2024

srnagar Jul 23, 2024

timotheeguerin Jul 23, 2024

m-nash Jul 22, 2024

iscai-msft Jul 22, 2024

srnagar Jul 23, 2024

srnagar Jul 23, 2024

timotheeguerin Jul 23, 2024

srnagar Jul 23, 2024

srnagar Jul 23, 2024

timotheeguerin Oct 17, 2024

timotheeguerin Oct 17, 2024

timotheeguerin Oct 17, 2024


		If you are adding a language-specific rule, you will also need this extra step 4. Add reference to the rule in the `[language]Rules` array in [`typespec-azure/packages/typespec-client-generator-core/src/linter.ts`][tcgc-linter]

		For Azure generations then, all rules, including all language-specific rules, will be run on the specs.


		Available ruleSets:

		- `@azure-tools/typespec-client-generator-core/all`

[tcgc] add linter rulesets framework #1208

Are you sure you want to change the base?

[tcgc] add linter rulesets framework #1208

Conversation

iscai-msft commented Jul 19, 2024 • edited Loading

azure-sdk commented Jul 19, 2024 • edited Loading

@azure-tools/typespec-client-generator-core - feature ✏️

@azure-tools/typespec-azure-rulesets - feature ✏️

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-sdk commented Jul 22, 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

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

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

iscai-msft commented Jul 19, 2024 •

edited

Loading

azure-sdk commented Jul 19, 2024 •

edited

Loading

`@azure-tools/typespec-client-generator-core` - feature ✏️

`@azure-tools/typespec-azure-rulesets` - feature ✏️