Skip to content

fix(eslint-plugin): [no-unnec-type-assert] handle JSX attributes#1002

Merged
bradzacher merged 11 commits intomasterfrom
tsx-test
Jan 13, 2020
Merged

fix(eslint-plugin): [no-unnec-type-assert] handle JSX attributes#1002
bradzacher merged 11 commits intomasterfrom
tsx-test

Conversation

@bradzacher
Copy link
Copy Markdown
Member

@bradzacher bradzacher commented Sep 23, 2019 •
edited
Loading

Fixes #982
Fixes #1061

I never added proper handling for JSX attributes in the rule - this fixes that.

This also fixes a few things with testing TSX files

  • the rule tester code (from the 2.0 changes) didn't allow you to change the filename
  • there was no tsx test file
  • there was weird behaviour from the tests if I didn't clean the caches between tests
    • it wasn't actually testing the tsx code for some reason.

@bradzacher bradzacher added the bug Something isn't working label Sep 23, 2019
@typescript-eslint

This comment has been minimized.

Sign in to view
@bradzacher bradzacher mentioned this pull request Oct 10, 2019
Closed
@bradzacher
Merge branch 'master' into tsx-test
d0b75a5 
# Conflicts:
#	.gitignore
@bradzacher bradzacher added the DO NOT MERGE PRs which should not be merged yet label Nov 20, 2019
armano2
armano2 reviewed Jan 11, 2020
View reviewed changes
Comment thread packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts
return <div key={props.id!} />;
};
`,
filename: path.join(rootDir, 'react.tsx'),
Copy link
Copy Markdown
Collaborator

@armano2 armano2 Jan 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

filename for now has to be absolute when used with watch program (project)

Copy link
Copy Markdown
Member Author

@bradzacher bradzacher Jan 12, 2020

Choose a reason for hiding this comment

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

do we want to put this in the RuleTester?

Copy link
Copy Markdown
Collaborator

@armano2 armano2 Jan 13, 2020

Choose a reason for hiding this comment

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

no it should not be there, it should work correctly with relative paths
see #1439

armano2
armano2 reviewed Jan 11, 2020
View reviewed changes
Comment thread packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts Outdated
armano2
armano2 previously requested changes Jan 11, 2020
View reviewed changes
Comment thread packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts Outdated
Comment thread packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts Outdated
Comment thread packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts Outdated
Comment thread packages/eslint-plugin/package.json Outdated
@armano2
Copy link
Copy Markdown
Collaborator

armano2 commented Jan 12, 2020 •
edited
Loading

@bradzacher i picked up this PR as it looked that you abandoned it (3 months ago last change),

i pushed merge + all conflicts are resolved and cleanup up code a little (most of changes was outdated and already supported on master)

i left some comments ^Up to show what i changed

@armano2 armano2 dismissed their stale review January 12, 2020 17:58

dismissed

@armano2 armano2 added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin and removed DO NOT MERGE PRs which should not be merged yet labels Jan 12, 2020
@armano2
Copy link
Copy Markdown
Collaborator

armano2 commented Jan 12, 2020

note: all changes related to #1061 are already present on master
df410a1#diff-9c31441197b8f835651744ab117e4b6dR223-R232

@armano2 armano2 mentioned this pull request Jan 12, 2020
Merged
@bradzacher
Copy link
Copy Markdown
Member Author

bradzacher commented Jan 12, 2020

Thanks, this was on my todo list. I had some commits locally, but never got around to finishing fixing it up.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 13, 2020 •
edited
Loading

Codecov Report

Merging #1002 into master will decrease coverage by 0.02%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #1002      +/-   ##
==========================================
- Coverage   94.48%   94.45%   -0.03%     
==========================================
  Files         142      142              
  Lines        6079     6083       +4     
  Branches     1727     1728       +1     
==========================================
+ Hits         5744     5746       +2     
- Misses        182      183       +1     
- Partials      153      154       +1
Impacted Files Coverage Δ
...pt-estree/src/create-program/createWatchProgram.ts 91.17% <100%> (-1.18%) ⬇️
packages/typescript-estree/src/node-utils.ts 96.98% <100%> (ø) ⬆️
packages/eslint-plugin-tslint/src/rules/config.ts 100% <100%> (ø) ⬆️
packages/typescript-estree/src/parser.ts 94.16% <100%> (+0.09%) ⬆️
...ges/typescript-estree/src/create-program/shared.ts 87.5% <80%> (+0.83%) ⬆️

@bradzacher bradzacher changed the title fix(eslint-plugin): [no-unnecessary-type-assertion] handle JSX attributes fix(eslint-plugin): [no-unnec-type-assert] handle JSX attributes Jan 13, 2020
@bradzacher bradzacher merged commit 3c5659b into master Jan 13, 2020
@bradzacher bradzacher deleted the tsx-test branch January 13, 2020 00:01
@snyk-bot snyk-bot mentioned this pull request Feb 8, 2020
Closed
@snyk-bot snyk-bot mentioned this pull request Feb 8, 2020
Closed
@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

1 more reviewer

@armano2 armano2 armano2 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Projects

None yet

Milestone

No milestone

2 participants

@bradzacher @armano2