[eslint-plugin-azure-sdk] Replaced redundant custom rule ts-no-namespaces with typescript-eslint/no-namespace #18676
Conversation
bbrusniak
requested review from
deyaaeldeen,
HarshaNalluru,
jeremymeng,
LarryOsterman,
mikeharder,
praveenkuttappan,
ramya-rao-a,
sadasant,
schaabs,
witemple-msft and
xirzec
as code owners
ghost
added
Azure.Core
Attestation
Container Registry
Event Hubs
Azure.Identity
eslint plugin
customer-reported
|
Thank you for your contribution bbrusniak! We will review the pull request and get back to you soon. |
|
|
There was a problem hiding this comment.
Nice. Can we turn on allowDeclarations in the rule's options? A lot of the places in the code where we disable this rule are not places where we're creating namespaces, but places where we're declaring types for modules or declaring global type definitions.
So I think declare global { ... } and declare module "foo" { ... } should be fine. We would only invoke these if there were things in the environment that we actually want to describe. I'm also fine with just requiring that we disable the rule in the rare cases that we want to use them, but I'm not sure if there are any cases where we would use a declare in error. @deyaaeldeen WDYT?
| namespace deleteasyncRetry { | ||
| export let internalCounter: number = 1; // eslint-disable-line prefer-const | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
/azp run js - identity - ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -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", | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| namespace deleteasyncRetry { | ||
| export let internalCounter: number = 1; // eslint-disable-line prefer-const | ||
| } |
There was a problem hiding this comment.
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.
I will log an issue for this |
This fixes #8717 I deleted the files implementing the ts-no-namespaces rule and enabled the typescript-eslint/no-namespace rule in the base config file. Also, updated files which reference ts-no-namespaces. Here is specifically what I did:
Let me know if I've missed anything in the removal of this rule as I am new to this code base!