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

For deprecated lifecycle methods, report identifier AST node instead of the class node #1854

Merged
merged 3 commits into from Jun 28, 2018

Conversation

jsnajdr
Copy link
Contributor

@jsnajdr jsnajdr commented Jun 26, 2018

When the no-deprecated rule finds a deprecated lifecycle method, it reports the whole class declaration as the AST node for the error. Doesn't look good in editor: makes the whole class declaration red and obscures all other ESLint errors inside the class.

screen shot 2018-06-26 at 11 53 22

This patch changes the reported AST node to the method name identifier. The errors are much better targeted at the infringing code and look much better when shown in editor UI.

screen shot 2018-06-26 at 11 29 01

Added also test coverage to check for the reported AST node type.

…he class node

When the `no-deprecated` rule finds a deprecated lifecycle method, it reports the whole
class declaration as the AST node for the error. Doesn't look good in editor and hides all
other ESLint errors inside the class.

This patch changes the reported AST node to the method name identifier. The errors are
much better targeted at the infringing code and look much better when shown in editor UI.

Added also test coverage to check for the reported AST node type.
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This seems great!

For the tests; could we add line and column info to all the error assertions, so we can prevent future issues?

@jsnajdr
Copy link
Contributor Author

jsnajdr commented Jun 26, 2018

@ljharb Thanks for reviewing this patch! I added a commit with line/column/endLine/endColumn checks.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2018

@jsnajdr looks like eslint 3 is failing on the endLine parts?

@jsnajdr
Copy link
Contributor Author

jsnajdr commented Jun 26, 2018

Oops, ESLint 3 apparently didn't support the endLine and endColumn fields yet. I removed them from the tests, leaving only line and column.

@ljharb ljharb merged commit f27285f into jsx-eslint:master Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants