Skip to content

rule no-import-test-files: modules that end in .js #221

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

rule no-import-test-files: modules that end in .js #221

wants to merge 6 commits into from

Conversation

Manu1400
Copy link

@Manu1400 Manu1400 commented Mar 24, 2019 •
edited by novemberborn
Loading

Fixes #215.

  • add new tests
  • use require.resolve() function

novemberborn
novemberborn requested changes Mar 31, 2019
View reviewed changes
Copy link
Member

@novemberborn novemberborn 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 @Manu1400! Looks like tests for this rule are failing, could you have another look?

rules/no-import-test-files.js
const multimatch = require('multimatch');
const util = require('../util');
const {getAvaConfig, defaultFiles, getDocsUrl} = require('../util');
Copy link
Member

Choose a reason for hiding this comment

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

Could you revert these changes?

@novemberborn
Copy link
Member

@Manu1400 looks like @GMartigny also took at try at solving this. His PR has fewer changes and the tests pass, would you mind having a look? #226.

For your next PR, it helps if you put something like "Fixes #215" in the PR description (I've added it above). That way it's more discoverable from the issue discussion.

@novemberborn
Copy link
Member

@Manu1400 just happened to notice this PR includes a fix for #213 as well (2d48704). Here too @GMartigny made a separate PR: #228. I like your approach better though, could you open a PR just for that change?

@sindresorhus sindresorhus mentioned this pull request May 14, 2019
Merged
@sindresorhus
Copy link
Member

Closing this in favor of #226.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@novemberborn novemberborn novemberborn requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

no-import-test-files incorrectly flags modules that end in .js
3 participants
@Manu1400 @novemberborn @sindresorhus