Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Adds no-dupe-args check to lint #99

Merged
merged 3 commits into from
Mar 3, 2020

Conversation

zinyando
Copy link
Contributor

@zinyando zinyando commented Mar 2, 2020

Adds a no-dupe-args check to lint on #94

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

CI linting is failing due to a bug in the new rule - https://github.com/facebookexperimental/rome/pull/99/checks?check_run_id=479521121

The implementation assumes that all function parameters are identifiers (type: 'Identifier') which is not always the case. Perhaps we can ignore the non-identifiers, which is what ESLint seems to be doing.

Please add tests as well. You can refer to the ESLint tests for the possible kinds of functions - https://github.com/eslint/eslint/blob/master/tests/lib/rules/no-dupe-args.js

@sebmck
Copy link
Contributor

sebmck commented Mar 2, 2020

This lint rule only works on regular name parameters, we can make it work more reliably, and support patterns with the getBindingIdentifiers method.

import {getBindingIdentifiers} from '@romejs/js-ast-utils';

The usage would be like this:

for (const param of node.params) {
  for (const {name} of getBindingIdentifiers(param)) {
    // logic here
  }
}

Then you can just maintain a single Set<string> with the checked arguments, then when found, produce a diagnostic.

Try to avoid the creation of duplicate AST arrays. For example, you used .filter(). Also avoid the usage of .forEach and instead use a regular for-of loop.

@sebmck
Copy link
Contributor

sebmck commented Mar 2, 2020

An example of the pattern I'm referring to is #98.

const uniqueIdentifiers = new Set();

for (const param of node.params) {
for (const {name} of getBindingIdentifiers(param)) {
Copy link
Contributor

@weblancaster weblancaster Mar 3, 2020

Choose a reason for hiding this comment

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

@zinyando would we be able to re-implement this algorithm by trying to target linear O(N) instead quadratic time O(N²)? unless these are really small and we shouldn't care about the performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't a more efficient way to do this. param could have multiple binding identifiers, which is what this will extract.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. thanks.

@sebmck
Copy link
Contributor

sebmck commented Mar 3, 2020

Thank you!

@sebmck sebmck merged commit c7ba615 into rome:master Mar 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants