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

@zephraph => [TSLint] Rule to warn about using QueryRenderer #3108

Merged
merged 1 commit into from
Jan 29, 2020

Conversation

mzikherman
Copy link
Collaborator

Starting out with a naive rule using just string comparison...will see if an AST if needed.

This is a WIP since it's still not actually working. Getting:

Error: Severity for rule 'noQueryRendererImport' not found

@artsy-peril artsy-peril bot added the Version: Patch Indicates that this PR should have a patch deploy, usually for bug fixes label Jan 24, 2020
tslint.json Outdated Show resolved Hide resolved
@mzikherman
Copy link
Collaborator Author

Thanks for the feedback! After editing the TSLint runner to avoid this severity error, I was able to get it to run, but of course the text based solution isn't going to cut it.

I was in the weeds since it also seems as though the way I'm specifying an error here, doesn't seem to quite work with our TSLint version, re: the missing severity thing. Was seeing some issues there: palantir/tslint#2650

But I think the AST-style rule will naturally return failures that are differently structured, which might avoid that issue altogether!

@zephraph zephraph added Version: Trivial This indicates that the PR does not need a deploy and removed Version: Patch Indicates that this PR should have a patch deploy, usually for bug fixes labels Jan 25, 2020
@codecov
Copy link

codecov bot commented Jan 29, 2020

Codecov Report

Merging #3108 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #3108   +/-   ##
======================================
  Coverage      82%     82%           
======================================
  Files         743     743           
  Lines       10883   10883           
  Branches     2182    2182           
======================================
  Hits         8933    8933           
  Misses       1365    1365           
  Partials      585     585

@mzikherman mzikherman changed the title @zephraph => [WIP] [TSLint] Rule to warn about using QueryRenderer @zephraph => [TSLint] Rule to warn about using QueryRenderer Jan 29, 2020
@mzikherman
Copy link
Collaborator Author

Assigning to @eessex - this adds a linter rule which fails (you can always disable the rule per line) if you use QueryRenderer (over the preferred SystemQueryRenderer).

The linter did locate one such location in Tooltips that was still using QueryRenderer, so made the recommended update (is that safe btw)?

Also cc @damassi I was expecting more violations but I think the sweep you did caught all of them! 👍 🙏 👏

Copy link
Contributor

@eessex eessex left a comment

Choose a reason for hiding this comment

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

I just pulled down this branch and checked out the tooltips, everything looks good from here 👍

@eessex eessex merged commit 57e8700 into master Jan 29, 2020
@eessex eessex deleted the lint_rule branch January 29, 2020 18:56
@artsyit
Copy link
Contributor

artsyit commented Jan 30, 2020

🚀 PR was released in v24.4.1 🚀

@damassi
Copy link
Member

damassi commented Feb 2, 2020

Nice @mzikherman 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Version: Trivial This indicates that the PR does not need a deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants