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.

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

[engsys][eslint-plugin] upgrade eslint dependencies #21733

Merged
merged 2 commits into from
May 18, 2022

Conversation

jeremymeng
Copy link
Member

@jeremymeng jeremymeng commented May 5, 2022

  • Fix broken test because of class property node type changes. Improve test a bit
    to also verifying error line and column numbers

  • Disable @typescript-eslint/no-invalid-this

After upgrading this rule reports more false positives. The following comment on the
github issue suggests that it doesn't work for some callbacks because it is just
a simple rule that doesn't do type analysis.

typescript-eslint/typescript-eslint#3620 (comment)

  • Fix linter issues after upgrading the plugins in eslint-plugin-azure-sdk. Also upgrade eslint version in the impacted packages

  - eslint to ^8.0.0
  - @types/eslint to ~8.4.0
  - @typescript-eslint plugins to ~5.22.0

* Fix broken test because of class property node type changes. Improve test a bit
to also verifying error line and column numbers

* Disable @typescript-eslint/no-invalid-this

After upgrading this rule reports more false positives. The following comment on the
github issue suggests that it doesn't work for some callbacks because it is just
a simple rule that doesn't do type analysis.

typescript-eslint/typescript-eslint#3620 (comment)

* Fix linter issues after upgrading the plugins in eslint-plugin-azure-sdk. Also upgrade eslint version in the impacted packages
@@ -17,7 +17,7 @@ function isDefined<T>(thing: T | undefined | null): thing is T {
* @param properties - The name of the properties that should appear in the object.
* @internal
*/
export function isObjectWithProperties<Thing extends unknown, PropertyName extends string>(
export function isObjectWithProperties<Thing, PropertyName extends string>(
Copy link
Member Author

Choose a reason for hiding this comment

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

opportunity for core-util

Copy link
Member Author

Choose a reason for hiding this comment

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

logged #21734

@azure-sdk
Copy link
Collaborator

API change check for @azure/app-configuration

API changes are not detected in this pull request for @azure/app-configuration

@azure-sdk
Copy link
Collaborator

API change check for @azure/service-bus

API changes are not detected in this pull request for @azure/service-bus

@azure-sdk
Copy link
Collaborator

API change check for @azure/event-hubs

API changes are not detected in this pull request for @azure/event-hubs

@azure-sdk
Copy link
Collaborator

API change check for @azure/monitor-query

API changes are not detected in this pull request for @azure/monitor-query

@azure-sdk
Copy link
Collaborator

API change check for @azure/core-amqp

API changes are not detected in this pull request for @azure/core-amqp

@azure-sdk
Copy link
Collaborator

API change check for @azure/core-auth

API changes are not detected in this pull request for @azure/core-auth

@azure-sdk
Copy link
Collaborator

API change check for @azure-rest/core-client

API changes are not detected in this pull request for @azure-rest/core-client

Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

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

Looks good! It is a bummer that no-invalid-this is not working properly though.

@jeremymeng
Copy link
Member Author

It is a bummer that no-invalid-this is not working properly though.

I will log an issue to re-visit this rule in case it got improved in the future, or if we missed anything.

@jeremymeng jeremymeng force-pushed the engsys/upgrade-eslint branch from c10b8cd to e63f32c Compare May 17, 2022 21:13
@jeremymeng
Copy link
Member Author

Bumping the rest of packages to use eslint@^8.0.0 works fine too. I will work with EngSys to merge directly to avoid triggering build storm.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Configuration Azure.ApplicationModel.Configuration Azure.Core eslint plugin Event Hubs Monitor Monitor, Monitor Ingestion, Monitor Query Service Bus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants