-
-
Notifications
You must be signed in to change notification settings - Fork 587
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
feat: add fixer
function to eslint/rules/no-internal-require
#3364
base: develop
Are you sure you want to change the base?
Conversation
@@ -83,9 +102,11 @@ function main( context ) { | |||
|
|||
rule = { | |||
'meta': { | |||
'type': 'suggestion', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heads up. Currently, the auto-fixing for suggestion
type is not supported.
This is an interesting one. I am not sure whether we should autofix this or not. If someone is reaching into a package for a particular file, we should probably just error. Auto-fixing could just lead to other issues. |
@kgryte do we currently have anything calling the internals in the codebase? Also what other issues the auto-fixing could cause? |
@headlessNode Not that I am aware of.
If someone is reaching internally into a package and we autofix without their explicit opt-in, that could lead to unexpected behavior. E.g., if someone is expecting to load a data JSON file, but gets the main export of a package instead because we rewrote The reason why I recommend just raising a lint "error" is that we effectively force devs to confront the fact that they are reaching into a package when we generally don't want that. Devs could always explicitly disable the lint rule if they are insistent, but that would be an intentional choice. |
@kgryte Makes sense. Should I close this PR then? |
Description
This pull request:
fixer
function toeslint/rules/no-internal-require
Related Issues
No.
Questions
No.
Other
No.
Checklist
@stdlib-js/reviewers