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

feat: @sapphire/eslint-plugin-result #556

Merged
merged 16 commits into from
Mar 12, 2023

Conversation

didinele
Copy link
Contributor

No description provided.

Copy link
Member

@favna favna left a comment

Choose a reason for hiding this comment

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

Also needs updating:

  • /.npm-deprecaterc.yml
  • I see that now we have to prefix the whole of @sapphire/eslint-plugin, whereas if the package would just be named eslint-plugin-result (which is in fact available) then it could just be result. Adding to my other comment about the usage that would also allow "extends": ["result/recommended"] iirc
    @vladfrangu wdyt about dropping our org prefix for this? Or alternatively eslint-plugin-sapphire-result.

Didn't review the actual code yet, just did this while waiting for a compile at work


Edit: So I looked up what the naming requirements are and with the current name we'd could do:

"plugins": ["@sapphire/result"]

See: https://eslint.org/docs/latest/use/configure/plugins#include-a-plugin

That has some appeal to it because it matches the actual package name, but at the same time I'm not if that very fact is a good idea. Again I defer to @vladfrangu for his input.

@vladfrangu
Copy link
Member

  • @vladfrangu wdyt about dropping our org prefix for this?

Not a fan of this, @favna. Also, doesn't using just @sapphire/result work too? (we should name this @sapphire/eslint-plugin-result too). I know it works for eslint rules (extends @sapphire)

@favna
Copy link
Member

favna commented Mar 5, 2023

  • @vladfrangu wdyt about dropping our org prefix for this?

Not a fan of this, @favna. Also, doesn't using just @sapphire/result work too? (we should name this @sapphire/eslint-plugin-result too). I know it works for eslint rules (extends @sapphire)

Further resolved on Discord. We're sticking to @sapphire/eslint-plugin-result.

@didinele didinele requested a review from favna March 10, 2023 20:01
@favna favna force-pushed the feat/result-eslint-rules branch 2 times, most recently from c346152 to 082f57e Compare March 10, 2023 22:48
Copy link
Member

@favna favna left a comment

Choose a reason for hiding this comment

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

There appear to still be some bugs in the plugin. You can repro the error with:

  1. Build all (required) packages with yarn build in the root folder
  2. Add to /.eslintrc extends: "plugin:@sapphire/result/recommended"
  3. cd packages/result
  4. Run yarn lint
  5. Observe the error:
Oops! Something went wrong! :(

ESLint: 8.28.0

TypeError: Cannot read properties of undefined (reading 'flags')
Occurred while linting /Users/favna/workspace/sapphire/utilities/packages/result/tests/Option.test.ts:832
Rule: "@sapphire/result/no-discard-result"
    at Object.getObjectFlags (/Users/favna/workspace/sapphire/utilities/node_modules/typescript/lib/typescript.js:19894:21)
    at couldContainTypeVariables (/Users/favna/workspace/sapphire/utilities/node_modules/typescript/lib/typescript.js:69657:34)
    at Object.some (/Users/favna/workspace/sapphire/utilities/node_modules/typescript/lib/typescript.js:748:25)
    at couldContainTypeVariables (/Users/favna/workspace/sapphire/utilities/node_modules/typescript/lib/typescript.js:69665:167)
    at instantiateTypeWithAlias (/Users/favna/workspace/sapphire/utilities/node_modules/typescript/lib/typescript.js:65125:18)
    at instantiateType (/Users/favna/workspace/sapphire/utilities/node_modules/typescript/lib/typescript.js:65122:37)
    at instantiateList (/Users/favna/workspace/sapphire/utilities/node_modules/typescript/lib/typescript.js:64712:34)
    at instantiateTypes (/Users/favna/workspace/sapphire/utilities/node_modules/typescript/lib/typescript.js:64726:20)
    at instantiateTypeWorker (/Users/favna/workspace/sapphire/utilities/node_modules/typescript/lib/typescript.js:65166:32)
    at instantiateTypeWithAlias (/Users/favna/workspace/sapphire/utilities/node_modules/typescript/lib/typescript.js:65139:26)

@didinele didinele requested a review from favna March 11, 2023 16:25
didinele and others added 14 commits March 12, 2023 15:27

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.

Unverified

This user has not yet uploaded their public signing key.
@favna favna force-pushed the feat/result-eslint-rules branch from 2b0c2c9 to b617753 Compare March 12, 2023 14:28

Unverified

This user has not yet uploaded their public signing key.
favna
favna previously approved these changes Mar 12, 2023

Unverified

This user has not yet uploaded their public signing key.
@favna favna merged commit 009d3d2 into sapphiredev:main Mar 12, 2023
@didinele didinele deleted the feat/result-eslint-rules branch March 12, 2023 18:52
goestav pushed a commit to goestav/utilities that referenced this pull request Jun 21, 2023

Partially verified

This commit is signed with the committer’s verified signature.
goestav’s contribution has been verified via GPG key.
We cannot verify signatures from co-authors, and some of the co-authors attributed to this commit require their commits to be signed.
* feat: eslint-plugin-result

* feat: support for void keyword

* chore: cleanup

* chore: typo

* chore: github meta

* chore: suggested changes

* refactor(recommended): apply plugin

* fix: ensure `eslint-plugin-result` is build before `ts-config`

* docs(readme): fixed usage for the plugin

* chore: regen lockfile

* chore: fix tsconfig for the plugin

* fix: hard crashes and lack of foo(bar()) handling

* ci: fix linting in CI

---------

Co-authored-by: Jeroen Claassens <support@favware.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants