Closed gaggle closed 4 years ago
Thanks for opening this issue!
not sure if you intend for "global" to also mean top-level in a module
Good question. To be honest I hadn't considered it either way. But now that you bring it up, yes I think having an arrow function in the outermost scope (whether that's the real global or whether it's the top-most scope of a module) should have been covered by the "global"
setting.
So, yes, it seems like it is a bug (in the sense of unintentional shortcoming).
To fix, yes, I think these lines need to be updated:
https://github.com/getify/eslint-plugin-proper-arrows/blob/master/lib/index.js#L730-L735
We'd probably best change them to:
["global","module",].includes(scope.upper.upper.type)
// ..
["global","module",].includes(scope.upper.type)
And for tests, we'd need to add these config presets at the top:
whereModuleGlobalDefault: {
parserOptions: { ecmaVersion: 2015, sourceType: "module", },
rules: { "@getify/proper-arrows/where": ["error",{property:false,export:false,trivial:true,},], },
},
whereModuleGlobal: {
parserOptions: { ecmaVersion: 2015, sourceType: "module", },
rules: { "@getify/proper-arrows/where": ["error",{global:true,property:false,export:false,trivial:true,},], },
},
Then we'd need these four tests at the end:
QUnit.test( "WHERE (module-global, default): conforming", function test(assert){
var code = `
function foo() {
var f = x => y;
}
`;
var results = eslinter.verify( code, linterOptions.whereModuleGlobalDefault );
assert.expect( 1 );
assert.strictEqual( results.length, 0, "no errors" );
} );
QUnit.test( "WHERE (module-global, default): violating", function test(assert){
var code = `
var f = x => y;
`;
var results = eslinter.verify( code, linterOptions.whereModuleGlobalDefault );
var [{ ruleId, messageId, } = {},] = results || [];
assert.expect( 3 );
assert.strictEqual( results.length, 1, "only 1 error" );
assert.strictEqual( ruleId, "@getify/proper-arrows/where", "ruleId" );
assert.strictEqual( messageId, "noGlobal", "messageId" );
} );
QUnit.test( "WHERE (module-global): conforming", function test(assert){
var code = `
function foo() {
var f = x => y;
}
`;
var results = eslinter.verify( code, linterOptions.whereModuleGlobal );
assert.expect( 1 );
assert.strictEqual( results.length, 0, "no errors" );
} );
QUnit.test( "WHERE (module-global): violating", function test(assert){
var code = `
var f = x => y;
`;
var results = eslinter.verify( code, linterOptions.whereModuleGlobal );
var [{ ruleId, messageId, } = {},] = results || [];
assert.expect( 3 );
assert.strictEqual( results.length, 1, "only 1 error" );
assert.strictEqual( ruleId, "@getify/proper-arrows/where", "ruleId" );
assert.strictEqual( messageId, "noGlobal", "messageId" );
} );
Also, of course, the docs need to be updated in several places of these sections:
https://github.com/getify/eslint-plugin-proper-arrows/blob/master/README.md#rule-configuration-1
Probably change all references from "top-level/global" to "module-top-level/global" or something like that.
As a side question, now maybe that "global"
name is confusing and should be changed to "outermost"
or "top"
? Not sure if it warrants changing, though. I guess most people will call a top-level module's scope "global" (or "module-global"). So probably don't need to change... but just something to ponder.
Thanks!, that was above and beyond clear :D Seems like a nice and easy repo to work with, well done.
A choice had to be made: An export violation is also a module-top-level violation, as I can see it that must be true by definition. To alter the public interface as little as possible I opted for a change such that export violation is tested first, and if one is found then it is not also reported as a global violation. Happy to change it to any direction you prefer, this just seemed to me to be the most intuitive.
An export violation is also a module-top-level violation, as I can see it that must be true by definition.
Good point, glad you brought it up.
I think the typical pattern I've used is that any overlapping rule is reported multiple times. In other words, a single line can have any number of rule violations applied to it. That's especially true since different rules might apply to different parts of the same line.
My reasoning was: it would be quite annoying to me if I found a line violating a linting rule, and then fixed that violation, only to have that same line now reported again for a different violation. Of course, this happens if my fix itself introduces the violation. But if the violation was hidden, I find that frustrating.
An export violation is also a module-top-level violation, as I can see it that must be true by definition.
Good point, glad you brought it up.
I think the typical pattern I've used is that any overlapping rule is reported multiple times. In other words, a single line can have any number of rule violations applied to it. That's especially true since different rules might apply to different parts of the same line.
My reasoning was: it would be quite annoying to me if I found a line violating a linting rule, and then fixed that violation, only to have that same line now reported again for a different violation. Of course, this happens if my fix itself introduces the violation. But if the violation was hidden, I find that frustrating.
Yeah that makes sense. I'll update the PR.
EDIT: Done, great precise feedback thanks
This is either a bug or I'm unwittingly making a feature-request.
I've added eslint-plugin-proper-arrows to our preset and added the following code to its test-suite which I was expecting to fail:
I expect it to generate two errors on rule
@getify/proper-arrows/where
, but actually only get one error for the arrow-export. My eslint configuration:From your readme I expected the arrow function to fail because it says:
Here top-level/global scope to me reads as the top-level in a module. But maybe that's not the intention of the library?
From debugging I think the change I expect could be satisfied by changing the line in https://github.com/getify/eslint-plugin-proper-arrows/blob/master/lib/index.js#L735 from:
to:
But I don't quite understand your test-suite setup (or indeed eslint in general :) so I'm not sure that's right.. and I'm doubly not sure if you intend for "global" to also mean top-level in a module at all. Maybe I'm actually making a feature-request to add a "module" toggle flag?
Either way I'm happy to try and help, but first need to reach out to you to understand the context of your library. Is this a bug at all?
(and thanks for making the library available)