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

Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) #13180

Closed
wants to merge 1 commit into from

Conversation

cmal
Copy link

@cmal cmal commented Apr 14, 2020

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

[X] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Fix the issue here: #12758

Is there anything you'd like reviewers to focus on?

  1. I thought two nodes are mergable only when one is ImportNamespaceSpecifier and one is ImportSpecifier, is that true?
  2. I thought only in handleImports when checking importsInFile should I change the checkAndReport function, is that true?

@jsf-clabot
Copy link

jsf-clabot commented Apr 14, 2020

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Apr 14, 2020
@aladdin-add aladdin-add added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Apr 16, 2020
@nzakas nzakas changed the title Fix: ignore unmergable imports when checking no-duplicate-imports Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) Jun 11, 2020
@nzakas
Copy link
Member

nzakas commented Jun 11, 2020

Sorry, looks like we missed this PR! @mdjermanovic you reopened the issue related to this PR, can you take a look?

@cmal
Copy link
Author

cmal commented Jun 29, 2020

👍

* Fix: typo in docs/rules/no-duplicate-imports.md
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left some notes.

In this PR we should also check how the new logic affects exports, fix #12760, and account for the new export * as ns syntax.

I'll try to make an overview on what should and what shouldn't be reported by this rule.

lib/rules/no-duplicate-imports.js Show resolved Hide resolved
Comment on lines +59 to +70
function isTwoNodesCanMerge(node1, node2) {
return (
(
checkImportType(node1, "ImportNamespaceSpecifier") &&
checkImportType(node2, "ImportSpecifier")
) ||
(
checkImportType(node1, "ImportSpecifier") &&
checkImportType(node2, "ImportNamespaceSpecifier")
)
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this function is actually doing the opposite of its name: checks if two nodes cannot be merged?

@nzakas
Copy link
Member

nzakas commented Jul 7, 2020

@cmal there are a couple of changes requested. Can you please take a look?

@cmal
Copy link
Author

cmal commented Sep 14, 2020

OK, sorry for late. I'll have a look.

boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request Mar 21, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request Mar 21, 2021
@mdjermanovic
Copy link
Member

Closing due to inactivity. New PR: #14238

boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request Mar 23, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
…slint#13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes eslint#12760)
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 9, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 23, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 23, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 23, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 23, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 23, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 27, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 27, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 27, 2021
boutahlilsoufiane added a commit to boutahlilsoufiane/eslint that referenced this pull request May 30, 2021
mdjermanovic pushed a commit that referenced this pull request Jun 4, 2021
…) (#14238)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes
 #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes
 #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes
 #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)

* Fix: ignore unmergable imports when checking no-duplicate-imports (fixes #13180) & Fix: Ignore re-export all in no-duplicate-imports (fixes #12760)
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Sep 19, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants