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

Add prefer-query-selector rule #198

Merged
merged 11 commits into from
Jan 14, 2019
Merged

Add prefer-query-selector rule #198

merged 11 commits into from
Jan 14, 2019

Conversation

janowsiany
Copy link
Contributor

fixes #171

@sindresorhus sindresorhus changed the title Add prefer-query-selector rule Add prefer-query-selector rule Oct 28, 2018
docs/rules/prefer-query-selector.md Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
rules/prefer-query-selector.js Outdated Show resolved Hide resolved

const VALID_QUOTES = /([',",`])/;
const getRange = (prop, node) => [prop.start, node.arguments[0].end];
const getReplacement = (ctx, identifierName, node) => {
Copy link
Owner

Choose a reason for hiding this comment

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

ctx => context

Generally, use readable names instead of abbreviations.

test/prefer-query-selector.js Outdated Show resolved Hide resolved
},
{
code: 'document.getElementsByClassName("foo bar");',
errors: [{message: 'Prefer `querySelector` over `getElementsByClassName`.'}],
Copy link
Owner

Choose a reason for hiding this comment

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

No need to repeat the error in every assertion. Just build them upfront, like done in https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/test/new-for-builtins.js

test/prefer-query-selector.js Outdated Show resolved Hide resolved
test/prefer-query-selector.js Show resolved Hide resolved
@sindresorhus
Copy link
Owner

The rule needs to be added to the bottom of the list here: https://github.com/sindresorhus/eslint-plugin-unicorn#rules

@sindresorhus
Copy link
Owner

@futpib @MrHen Would you be able to help review? :)

rules/prefer-query-selector.js Outdated Show resolved Hide resolved
rules/prefer-query-selector.js Outdated Show resolved Hide resolved
test/prefer-query-selector.js Show resolved Hide resolved
test/prefer-query-selector.js Show resolved Hide resolved
rules/prefer-query-selector.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

@janowsiany Ping :)

@janowsiany
Copy link
Contributor Author

janowsiany commented Dec 29, 2018

@sindresorhus sorry for such long delay, please revalidate. I decided to make it partly fixable only because i found template strings and other corner cases hard or impossible to fix.

@sindresorhus sindresorhus merged commit a44e16c into sindresorhus:master Jan 14, 2019
@sindresorhus
Copy link
Owner

This looks good to me now. Very nice work, @janowsiany 👍

@janowsiany janowsiany deleted the prefer-query-selector branch January 14, 2019 07:18
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.

Rule proposal: prefer-query-selector
4 participants