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

chore: fix lint warnings in KeyringController #5170

Merged
merged 9 commits into from
Feb 3, 2025
Merged

Conversation

mikesposito
Copy link
Member

Explanation

Lint warnings in the KeyringController package have been fixed

References

N/A

Changelog

No consumer-facing changes

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

When you run yarn lint do you see changes to eslint-warning-thresholds.json? If so can you commit those too? You should see numbers go down.

*
* Keyring object to return in fullUpdate
* @property type - Keyring type
Copy link
Member

Choose a reason for hiding this comment

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

Nit: While @property was certainly not the proper way to document type properties (it's not supported by TSdoc, TypeDoc, or VSCode), we can document them like this:

export type KeyringObject = {
  /**
   * Associated accounts
   */
  accounts: string[];
  /**
   * Keyring type
   */
  type: string;
};

It is nice to have docs for all properties, certainly if they're already documented. I see a few others were deleted in this PR as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I added some descriptions on KeyringObject and KeyringControllerState: 1334e78

@mikesposito
Copy link
Member Author

mikesposito commented Jan 29, 2025

When you run yarn lint do you see changes to eslint-warning-thresholds.json? If so can you commit those too? You should see numbers go down.

@mcmire I updated the file, but now yarn lint is failing on the CI while its working fine on local 🤔 any idea why this is happening?

@mcmire
Copy link
Contributor

mcmire commented Jan 30, 2025

@mikesposito Hmm. I am able to replicate the error that CI shows.

Maybe your ESLint cache is out of date? Try running: yarn build:only-clean && yarn ts-node ./scripts/run-eslint.ts (this is the same as the yarn lint:eslint command but minus --cache).

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@ccharly ccharly left a comment

Choose a reason for hiding this comment

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

Much better now, thanks!

@@ -2201,9 +2194,6 @@ export class KeyringController extends BaseController<
// NOTE: Not all keyrings implement this method in a asynchronous-way. Using `await` for
// non-thenable will still be valid (despite not being really useful). It allows us to cover both
// cases and allow retro-compatibility too.
// FIXME: For some reason, it seems that eslint is complaining about this call being non-thenable
// even though it is... For now, we just disable it:
// eslint-disable-next-line @typescript-eslint/await-thenable
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks for fixing/removing this one 😄 This drove me crazy last time! Might have been a false-positive and the eslint update fixed it!

@mikesposito mikesposito merged commit 9b41ea4 into main Feb 3, 2025
127 checks passed
@mikesposito mikesposito deleted the mikesposito/lint-fix branch February 3, 2025 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants