Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Azure Core: Enable no-enum rule by default in all ruleset #467

Merged
merged 7 commits into from
Mar 22, 2024
Merged

Azure Core: Enable no-enum rule by default in all ruleset #467

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8301977
enable no-enum rule
timotheeguerin Mar 21, 2024
6d66c5d
Merge branch 'main' into enable-no-enum-rule
timotheeguerin Mar 21, 2024
85fb7f3
Create enable-no-enum-rule-2024-2-21-16-42-36.md
timotheeguerin Mar 21, 2024
78b66c9
fix rule
timotheeguerin Mar 21, 2024
35d1467
Update .chronus/changes/enable-no-enum-rule-2024-2-21-16-42-36.md
timotheeguerin Mar 21, 2024
447a7c7
Merge branch 'main' into enable-no-enum-rule
timotheeguerin Mar 21, 2024
3c5ff68
Merge branch 'main' into enable-no-enum-rule
timotheeguerin Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .chronus/changes/enable-no-enum-rule-2024-2-21-16-42-36.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
# Change versionKind to one of: internal, fix, dependencies, feature, deprecation, breaking
changeKind: fix
timotheeguerin marked this conversation as resolved.
Show resolved Hide resolved
packages:
- "@azure-tools/typespec-azure-core"
---

Enable `no-enum` rule by default in `all` ruleset
2 changes: 1 addition & 1 deletion packages/typespec-azure-core/src/linter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export const $linter = defineLinter({
true,
[`@azure-tools/typespec-azure-core/${useStandardNames.name}`]: true,
[`@azure-tools/typespec-azure-core/${friendlyNameRule.name}`]: true,
[`@azure-tools/typespec-azure-core/${noEnumRule.name}`]: false,
[`@azure-tools/typespec-azure-core/${noEnumRule.name}`]: true,
},
extends: ["@typespec/http/all"],
},
Expand Down
4 changes: 3 additions & 1 deletion packages/typespec-azure-core/src/rules/no-enum.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ export const noEnumRule = createRule({
create(context) {
return {
enum: (en: Enum) => {
if (getVersionsForEnum(context.program, en).length > 0) {
const [_, versions] = getVersionsForEnum(context.program, en);
if (versions !== undefined && versions.getVersions()[0].enumMember.enum === en) {
return;
}

context.reportDiagnostic({
format: { enumName: en.name },
target: en,
Expand Down
22 changes: 22 additions & 0 deletions packages/typespec-azure-core/test/rules/no-enum.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ describe("typespec-azure-core: no-enum rule", () => {
},
]);
});

it("allows the version enum", async () => {
await tester
.expect(
Expand All @@ -46,6 +47,27 @@ describe("typespec-azure-core: no-enum rule", () => {
.toBeValid();
});

it("emit warning about other enums in versioned service", async () => {
Copy link
Member

@MaryGao MaryGao Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering why we have this warning in azure scope?

Looking at the swagger, there are a lot of cases where "modelAsString": false in enum extensible and I think they would be taken as fixed.
https://github.com/Azure/azure-rest-api-specs/blob/feature/health-lro/specification/compute/resource-manager/Microsoft.Compute/CloudserviceRP/preview/2020-10-01-preview/cloudService.json#L1483-L1492

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being an azure scope is the entire point of this linting rule. Actual enums cannot deal with new values being added so azure policy is you shouldn’t use them. There is of course always exceptions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this guideline: https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#enums--sdks-client-libraries, is it from there?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

await tester
.expect(
`
@service
@versioned(Versions)
namespace Foo;
enum Versions {
v1, v2
}

enum Bar { a, b}
`
)
.toEmitDiagnostics([
{
code: "@azure-tools/typespec-azure-core/no-enum",
},
]);
});

describe("codefix", () => {
it("codefix simple enum", async () => {
await tester
Expand Down
Loading