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

[eslint-plugin-azure-sdk] Replaced redundant custom rule ts-no-namespaces with typescript-eslint/no-namespace #18676

Merged
merged 3 commits into from
Nov 22, 2021
Merged

[eslint-plugin-azure-sdk] Replaced redundant custom rule ts-no-namespaces with typescript-eslint/no-namespace #18676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f61a1ac
Removed ts-no-namespaces and replaced with typescript-eslint/no-names…
Nov 14, 2021
e66e66e
Merge branch 'main' into issue-8717
Nov 14, 2021
97e8887
Fixed README.md
Nov 14, 2021
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
1 change: 0 additions & 1 deletion common/tools/eslint-plugin-azure-sdk/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ Some rules (see table below) are fixable using the `--fix` ESLint option (added
| [**ts-naming-subclients**](https://github.com/Azure/azure-sdk-for-js/blob/main/common/tools/eslint-plugin-azure-sdk/docs/rules/ts-naming-subclients.md) | :triangular_flag_on_post: | :x: | `1.2.0` |
| [**ts-no-const-enums**](https://github.com/Azure/azure-sdk-for-js/blob/main/common/tools/eslint-plugin-azure-sdk/docs/rules/ts-no-const-enums.md) | :warning: | :heavy_check_mark: | `1.1.0` |
| [**ts-no-window**](https://github.com/Azure/azure-sdk-for-js/blob/main/common/tools/eslint-plugin-azure-sdk/docs/rules/ts-no-window.md) | :triangular_flag_on_post: | :heavy_check_mark: | `3.1.0` |
| [**ts-no-namespaces**](https://github.com/Azure/azure-sdk-for-js/blob/main/common/tools/eslint-plugin-azure-sdk/docs/rules/ts-no-namespaces.md) | :triangular_flag_on_post: | :x: | `1.2.0` |
| [**ts-package-json-author**](https://github.com/Azure/azure-sdk-for-js/blob/main/common/tools/eslint-plugin-azure-sdk/docs/rules/ts-package-json-author.md) | :triangular_flag_on_post: | :heavy_check_mark: | `1.0.0` |
| [**ts-package-json-bugs**](https://github.com/Azure/azure-sdk-for-js/blob/main/common/tools/eslint-plugin-azure-sdk/docs/rules/ts-package-json-bugs.md) | :triangular_flag_on_post: | :heavy_check_mark: | `1.0.0` |
| [**ts-package-json-engine-is-present**](https://github.com/Azure/azure-sdk-for-js/blob/main/common/tools/eslint-plugin-azure-sdk/docs/rules/ts-package-json-engine-is-present.md) | :triangular_flag_on_post: | :heavy_check_mark: | `1.1.0` |
Expand Down
21 changes: 0 additions & 21 deletions common/tools/eslint-plugin-azure-sdk/docs/rules/ts-no-namespaces.md
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion common/tools/eslint-plugin-azure-sdk/src/configs/azure-sdk-base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ export default {
"@typescript-eslint/no-inferrable-types": "off",
// We use empty extends and empty interface for shimming and renaming extensively
"@typescript-eslint/no-empty-interface": "off",
"@typescript-eslint/no-namespace": "off",
"@typescript-eslint/no-namespace": "error",
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why it was turned off in the first place? but this is a question to the team :) @witemple-msft and @sadasant do you know?

Copy link
Member

Choose a reason for hiding this comment

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

We had our own namespace rule which this PR removes in favor of this built-in one.

In the history of our eslint plugin, it was basically developed in 1:1 correspondence with the guidelines, and at that time the eslint/tslint/typescript-eslint story was not as set in stone as it is now where pretty much everyone is using eslint with the typescript plugin.

"@typescript-eslint/no-non-null-assertion": "off",
"@typescript-eslint/no-unused-vars": ["warn", { argsIgnorePattern: "^_" }],
"@typescript-eslint/no-useless-constructor": "error",
Expand Down
1 change: 0 additions & 1 deletion common/tools/eslint-plugin-azure-sdk/src/configs/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ export = {
"@azure/azure-sdk/ts-naming-options": "error",
"@azure/azure-sdk/ts-naming-subclients": "error",
"@azure/azure-sdk/ts-no-const-enums": "warn",
"@azure/azure-sdk/ts-no-namespaces": "error",
"@azure/azure-sdk/ts-no-window": "error",
"@azure/azure-sdk/ts-package-json-author": "error",
"@azure/azure-sdk/ts-package-json-bugs": "error",
Expand Down
2 changes: 0 additions & 2 deletions common/tools/eslint-plugin-azure-sdk/src/rules/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import tsNamingDropNoun from "./ts-naming-drop-noun";
import tsNamingOptions from "./ts-naming-options";
import tsNamingSubclients from "./ts-naming-subclients";
import tsNoConstEnums from "./ts-no-const-enums";
import tsNoNamespaces from "./ts-no-namespaces";
import tsNoWindow from "./ts-no-window";
import tsPackageJsonAuthor from "./ts-package-json-author";
import tsPackageJsonBugs from "./ts-package-json-bugs";
Expand Down Expand Up @@ -80,7 +79,6 @@ export = {
"ts-naming-options": tsNamingOptions,
"ts-naming-subclients": tsNamingSubclients,
"ts-no-const-enums": tsNoConstEnums,
"ts-no-namespaces": tsNoNamespaces,
"ts-no-window": tsNoWindow,
"ts-package-json-author": tsPackageJsonAuthor,
"ts-package-json-sdktype": tsPackageJsonSdkType,
Expand Down
30 changes: 0 additions & 30 deletions common/tools/eslint-plugin-azure-sdk/src/rules/ts-no-namespaces.ts
View file
Open in desktop

This file was deleted.

48 changes: 0 additions & 48 deletions common/tools/eslint-plugin-azure-sdk/tests/rules/ts-no-namespaces.ts
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion sdk/attestation/attestation/src/jsrsasign.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

// eslint-disable-next-line @azure/azure-sdk/ts-no-namespaces
// eslint-disable-next-line @typescript-eslint/no-namespace
declare module 'jsrsasign';
2 changes: 1 addition & 1 deletion sdk/containerregistry/container-registry/src/utils/base64.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

// eslint-disable-next-line @azure/azure-sdk/ts-no-namespaces
// eslint-disable-next-line @typescript-eslint/no-namespace
declare global {
function atob(input: string): string;
}
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/core-client/src/base64.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

// eslint-disable-next-line @azure/azure-sdk/ts-no-namespaces
// eslint-disable-next-line @typescript-eslint/no-namespace
declare global {
// stub these out for the browser
function btoa(input: string): string;
Expand Down
2 changes: 1 addition & 1 deletion sdk/core/core-http/src/httpOperationResponse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export interface HttpResponse {
headers: HttpHeadersLike;
}

// eslint-disable-next-line @azure/azure-sdk/ts-no-namespaces
// eslint-disable-next-line @typescript-eslint/no-namespace
declare global {
/**
* Stub declaration of the browser-only Blob type.
Expand Down
6 changes: 3 additions & 3 deletions sdk/core/core-lro/test/utils/router/paramRoutes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ function deleteasyncRetry(request: PipelineRequest): PipelineResponse | undefine
return undefined;
}

// eslint-disable-next-line @azure/azure-sdk/ts-no-namespaces
// eslint-disable-next-line @typescript-eslint/no-namespace
namespace deleteasyncRetry {
export let internalCounter: number = 1; // eslint-disable-line prefer-const
}
Comment on lines 228 to 230
Copy link
Member

Choose a reason for hiding this comment

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

This is neither here nor there for this PR but the namespaces in these files are the only namespace declarations that actually stick out to me. CC @sadasant and @deyaaeldeen should we file an issue to rework these?

Copy link
Member

Choose a reason for hiding this comment

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

I was trying to mimic static variables based on this recommendation: microsoft/TypeScript#8419 (comment) and did not want to change the whole setup of routes logic as functions to accommodate stateful ones.

I am fine with opening an issue though to track updating this.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see the purpose of the pattern now is to let the namespace merge with the function and bind those static fields to it that way. It would be nice to find a different way to bind these to instances of the function (using Object.assign and explicitly giving the function's name an extended type might work, or even a class that extends Function), but it doesn't matter too much.

Expand Down Expand Up @@ -293,7 +293,7 @@ function putasyncRetry(request: PipelineRequest): PipelineResponse | undefined {
return undefined;
}

// eslint-disable-next-line @azure/azure-sdk/ts-no-namespaces
// eslint-disable-next-line @typescript-eslint/no-namespace
namespace putasyncRetry {
export let internalCounter: number = 1; // eslint-disable-line prefer-const
}
Expand Down Expand Up @@ -369,7 +369,7 @@ function postasyncRetry(request: PipelineRequest): PipelineResponse | undefined
return undefined;
}

// eslint-disable-next-line @azure/azure-sdk/ts-no-namespaces
// eslint-disable-next-line @typescript-eslint/no-namespace
namespace postasyncRetry {
export let internalCounter: number = 1; // eslint-disable-line prefer-const
}
Expand Down
2 changes: 1 addition & 1 deletion sdk/eventhub/event-hubs/src/connectionContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

/* eslint-disable @azure/azure-sdk/ts-no-namespaces */
/* eslint-disable @typescript-eslint/no-namespace */
/* eslint-disable no-inner-declarations */

import { logger, logErrorStackTrace } from "./log";
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/identity/test/internal/node/interactiveBrowserCredential.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Licensed under the MIT license.

/* eslint-disable @typescript-eslint/no-non-null-asserted-optional-chain */
/* eslint-disable @azure/azure-sdk/ts-no-namespaces */
/* eslint-disable @typescript-eslint/no-namespace */

import Sinon from "sinon";
import { assert } from "chai";
Expand Down