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] newline-after-import: fix exactCount with considerComments false positive, when there is a leading comment #2884

Merged

Conversation

kinland
Copy link
Contributor

@kinland kinland commented Sep 26, 2023

This addresses #2882 and adds additional test cases to cover the combination of exactCount and considerComments

…` false positive, when there is a leading comment

Fixes import-js#2882.
@kinland kinland force-pushed the issue-2882-exact-count-with-consider-comments branch 2 times, most recently from dbd4ad7 to ef3d0e7 Compare September 26, 2023 02:22
@kinland
Copy link
Contributor Author

kinland commented Sep 26, 2023

Sorry about the force-pushes... I'm a bit sleep deprived

@kinland
Copy link
Contributor Author

kinland commented Sep 26, 2023

I found an edge case that I missed... will push a new commit when I figure out how to fix it.

tests/src/rules/newline-after-import.js Outdated Show resolved Hide resolved
@kinland kinland requested a review from ljharb September 26, 2023 03:30
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.

LGTM assuming the new tests fail without the change, and pass with them. Thanks!

@ljharb ljharb force-pushed the issue-2882-exact-count-with-consider-comments branch from 06e8759 to 66cb10f Compare September 26, 2023 04:18
@ljharb ljharb changed the title [Fix] #2882 exactCount with considerComments false positive [Fix] newline-after-import: fix exactCount with considerComments false positive, when there is a leading comment Sep 26, 2023
@ljharb ljharb merged commit 66cb10f into import-js:main Sep 26, 2023
164 of 166 checks passed
@reosarevok
Copy link
Contributor

Thanks a lot! Clearly my one added test wasn't good enough when rebasing the previous PR, sorry.

@kinland
Copy link
Contributor Author

kinland commented Sep 27, 2023

Thanks a lot! Clearly my one added test wasn't good enough when rebasing the previous PR, sorry.

Honestly, I can't fault you for that. I didn't consider that a comment before an import would break my code, either. These things get very complex quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants