-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 fixer for first
#1046
add fixer for first
#1046
Conversation
, output: "import { x } from './foo';\ | ||
import { y } from './bar';\ | ||
import { z } from './baz';\ | ||
export { x };" | ||
}) | ||
, test({ code: "import { x } from './foo'; import { y } from 'bar'" |
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.
can we validate that the output matches the input here?
tests/src/rules/first.js
Outdated
, errors: 3 | ||
, output: "import { x } from './foo';\ | ||
import { y } from './bar';\ | ||
import { z } from './baz';\nvar foo = bar;" |
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.
nbd but could this use a continuation instead of a literal \n
?
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.
it must be a seperator here. I thought a line-breaker is suitable, or may be another option?
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.
It just looks odd to see the previous lines using \ but this one using \n
tests/src/rules/first.js
Outdated
import { z } from './baz';" | ||
, errors: 2 | ||
, output: "import { y } from './bar';\ | ||
import { z } from './baz';\nvar a = x;\ |
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.
this is actually incorrect; because foo
might depend on side effects from bar
and baz
- this example can't be reordered at all.
tests/src/rules/first.js
Outdated
import { y } from './bar';\ | ||
import { z } from './baz';" | ||
, errors: 3 | ||
, output: "import { x } from './foo';\ |
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.
same here.
Are you no longer interested in completing this PR? (If so, please reopen this one instead of opening another) |
now if a specifier is referenced before the import, it won't be reordered. |
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.
I don't know anything about fixers but the tests look good 👍
@fengkfengk now we just need to get coverage metrics back up, and we can merge this! |
now the coverage improved, some unreachable lines are fixed. |
Add autofixer for rule
first
. Refer issue #1040.