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 import-style rule #789

Merged
merged 23 commits into from
Jul 30, 2020
Merged

Add import-style rule #789

merged 23 commits into from
Jul 30, 2020

Conversation

futpib
Copy link
Contributor

@futpib futpib commented Jul 3, 2020

fixes #211


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


@futpib futpib force-pushed the import-style branch 3 times, most recently from eed1682 to 899c96f Compare July 3, 2020 18:32
@futpib futpib changed the title Import style Add import-style rule Jul 3, 2020
@futpib futpib marked this pull request as ready for review July 3, 2020 20:50
@fisker
Copy link
Collaborator

fisker commented Jul 4, 2020

Are we going to allow check `./util'?

@futpib
Copy link
Contributor Author

futpib commented Jul 6, 2020

I would prefer not to. If we are to support relative paths properly, we are to invite a lot of complexities of eslint-plugin-import (pluggable resolvers). (EDIT: I'm probably wrong about this) I'd rather leave it as undefined behaviour (which it currently is), advise against it in the docs or throw if a relative path is found in options.

@futpib futpib force-pushed the import-style branch 3 times, most recently from 9e2dc29 to 2a02047 Compare July 7, 2020 17:29
@manovotny
Copy link
Contributor

I stumbled across this while looking for a way to enforce the use of const / require instead of accidental import for apps / packages that haven't opted into the experimental support in newer node versions yet.

Do y'all think this would be something appropriate to add to this import-style rule or something completely different? I also didn't see anything else out there in native eslint, but please forgive me if I overlooked something.

@futpib futpib requested a review from fisker July 17, 2020 16:26
rules/import-style.js Outdated Show resolved Hide resolved
docs/rules/import-style.md Outdated Show resolved Hide resolved
rules/import-style.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

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

I'll take a deep look ASAP.

Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

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

Looks great ,

  1. I didn't see import chalk, {red } from test
  2. We agree to check export from in previous PR

rules/import-style.js Show resolved Hide resolved
rules/import-style.js Show resolved Hide resolved
@futpib
Copy link
Contributor Author

futpib commented Jul 18, 2020

@fisker I added export ... from ... support but chose to leave it disabled by default (behind checkExportFrom option).

The original motivation for this rule (as far as I understand) was to force the named import of unrelated functions and default or namespace import of related functions. export ... from ... allows you to turn unrelated-functions-module (like util) into a related-functions-module (like chalk) like this:

export {promisify, callbackify} from 'util';

I think the above code should not be reported. @sindresorhus, hope this decision is fine by you.

@futpib futpib requested a review from fisker July 22, 2020 01:38
Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

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

All good, 2 notes:

  1. I suggest apply this, but it's only a style issue
    Add import-style rule #789 (comment)

  2. Add a test about this, I can accept any result about it
    Add import-style rule #789 (comment)

@futpib
Copy link
Contributor Author

futpib commented Jul 30, 2020

I addressed the remaining review comments.

@sindresorhus sindresorhus merged commit 0c7a199 into sindresorhus:master Jul 30, 2020
@sindresorhus
Copy link
Owner

Great work on this, @futpib 👍🏻

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: Don't import util Node.js core module directly
4 participants