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

Jump to bottom

chore: enable prefer-ast-types-enum internal rule #1514

Merged
merged 12 commits into from Jan 25, 2020
Merged

chore: enable prefer-ast-types-enum internal rule #1514

merged 12 commits into from Jan 25, 2020

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jan 24, 2020

This is based off #1508, so shouldn't be merged until after that one has.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @G-Rath!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 24, 2020 •
edited

Looking over the diff, there are some pretty decent ones in there that have been caught & fixed.

constructor is the only "ehhh" one that looks like it could be considered overzealous.

@G-Rath @bradzacher
chore(eslint-plugin-internal): use hasOwnProperty instead of in
9fa1750 
Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>
@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 24, 2020

@armano2 now this should be good & ready for review :)

@G-Rath G-Rath requested a review from armano2 January 24, 2020 22:09
bradzacher
bradzacher requested changes Jan 24, 2020
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

Almost all changes are correct.
If you undo the changes to the files I've marked, and disable with a comment, I'm happy to merge this.

We could probs improve it with type info, but it's probs not worth it. The cases are pretty edge-casey, and fine for the internal usage.
The rule in #315 will cover this so I'm happy with a few disable comments until we get around to finishing that rule.

I should probably prioritise finishing some of those "abandoned" PRs.

packages/eslint-plugin/tests/rules/ban-types.test.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-unused-vars-experimental.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-type-alias.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/no-inferrable-types.ts Outdated Show resolved Hide resolved
@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 24, 2020

👍

I should probably prioritise finishing some of those "abandoned" PRs.

I'm happy to help out with this - I've already got a couple on my plate that I'd like to action but they're all somewhat complicated or have high depth, so I'm keen for some lighter-weight contributions to sink my teeth into for upskilling 🙂

(In particular, learning how to use the types provided by TypeScript properly)

G-Rath
G-Rath commented Jan 24, 2020
View reviewed changes
packages/eslint-plugin-internal/tests/rules/prefer-ast-types-enum.test.ts
`,
errors: [
{
data: { enumName: 'AST_NODE_TYPES', literal: AST_NODE_TYPES.Literal },
Copy link
Contributor Author

@G-Rath G-Rath Jan 24, 2020 •
edited

Choose a reason for hiding this comment

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

ah haha hoisted by my own petard!

@armano2

This comment has been minimized.

Sign in to view
@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 25, 2020

what do you think about changing this validation to something like this

I think that while cool it's probably worth focusing our efforts on getting #315 across the line before iterating any further on this rule, as I think we've got the majority of the false false positives, and it'll also make for a good double-blind test for that rule :)

armano2
armano2 previously approved these changes Jan 25, 2020
View reviewed changes
@G-Rath G-Rath requested a review from armano2 January 25, 2020 00:59
armano2
armano2 approved these changes Jan 25, 2020
View reviewed changes
Copy link
Member

@armano2 armano2 left a comment

Choose a reason for hiding this comment

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

LGTM

bradzacher
bradzacher approved these changes Jan 25, 2020
View reviewed changes
Copy link
Member

@bradzacher bradzacher left a comment •
edited

Choose a reason for hiding this comment

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

legendary, thanks!

@bradzacher bradzacher changed the title Enable prefer ast types internal rule chore: enable prefer-ast-types-enum internal rule Jan 25, 2020
@bradzacher bradzacher added the tests anything to do with testing label Jan 25, 2020
@bradzacher bradzacher merged commit afa7900 into typescript-eslint:master Jan 25, 2020
@G-Rath G-Rath deleted the enable-prefer-ast-types-internal-rule branch January 25, 2020 04:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@armano2 armano2 armano2 approved these changes

@bradzacher bradzacher bradzacher approved these changes

Assignees
No one assigned
Labels
tests anything to do with testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@G-Rath @armano2 @bradzacher