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.

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

Fix: Adds conditional for separateRequires in one-var (fixes #10179) #10980

Merged
merged 1 commit into from Feb 15, 2019
+25 −2
Merged

Fix: Adds conditional for separateRequires in one-var (fixes #10179) #10980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/rules/one-var.js
View file
Open in desktop
Expand Up @@ -257,7 +257,9 @@ module.exports = {

if (currentOptions.uninitialized === MODE_ALWAYS && currentOptions.initialized === MODE_ALWAYS) {
if (currentScope.uninitialized || currentScope.initialized) {
return false;
if (!hasRequires) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this also be using separateRequires? I can't do a deep dive at the moment, but this caught my eye.

Copy link
Contributor Author

@sstern6 sstern6 Nov 12, 2018 •
edited

Choose a reason for hiding this comment

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

instead of hasRequires? or in addition to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaicataldo could you expand on your question, dont think I understand what youre asking.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, got a bit busy and lost track of this. My question was about these checks always checking if there are requires but not checking if the separateRequires config option was set or not. Hopefully I'll be able to look at this a little closer later tonight.

Copy link
Contributor Author

@sstern6 sstern6 Nov 15, 2018 •
edited

Choose a reason for hiding this comment

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

@kaicataldo I see. Good point. The false being returned in my contributions allow this conditional to be fulfilled.

eslint/lib/rules/one-var.js

Line 441 in 56a677c

if (!hasOnlyOneStatement(type, declarations)) {
. Though thats a good point, for this ticket i could add options.separateRequires && !hasRequires.

WDYT?

Edit: My recommendation seems to break over 20 tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaicataldo bump

return false;
}
}
}

Expand All @@ -268,7 +270,9 @@ module.exports = {
}
if (declarationCounts.initialized > 0) {
if (currentOptions.initialized === MODE_ALWAYS && currentScope.initialized) {
return false;
if (!hasRequires) {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

return false;
}
}
}
if (currentScope.required && hasRequires) {
Expand Down
19 changes: 19 additions & 0 deletions tests/lib/rules/one-var.js
View file
Open in desktop
Expand Up @@ -209,6 +209,14 @@ ruleTester.run("one-var", rule, {
options: [{ separateRequires: true, var: "always" }],
parserOptions: { env: { node: true } }
},
{
code: "var bar = 'bar'; var foo = require('foo');",
options: [{ separateRequires: true, var: "always" }]
},
{
code: "var foo = require('foo'); var bar = 'bar';",
options: [{ separateRequires: true, var: "always" }]
},
{
code: "var foo = require('foo'); var bar = 'bar';",
options: [{ separateRequires: true, var: "always" }],
Expand Down Expand Up @@ -1092,6 +1100,17 @@ ruleTester.run("one-var", rule, {
column: 25
}]
},
{
code: "var a = true; var b = false;",
output: "var a = true, b = false;",
aladdin-add marked this conversation as resolved.
Show resolved Hide resolved
options: [{ separateRequires: true, var: "always" }],
errors: [{
message: "Combine this with the previous 'var' statement.",
type: "VariableDeclaration",
line: 1,
column: 15
}]
},
{
code: "const a = 0; let b = 1; let c = 2; const d = 3;",
output: "const a = 0; let b = 1, c = 2; const d = 3;",
Expand Down