Fix: Make MemberExpression option opt-in (fixes #6797) #6798
Conversation
|
LGTM |
|
LGTM |
|
But this should be for #6795? If I understand you correctly |
|
Fix for 6795 is "ignore assignment statements". Fix for 6797 is "make it
|
|
But both PRs are sent to fix one "breaking change", but okay, I understand the logic behind it |
| @@ -97,7 +97,7 @@ module.exports = { | |||
| const: DEFAULT_VARIABLE_INDENT | |||
| }, | |||
| outerIIFEBody: null, | |||
| MemberExpression: 1 | |||
| MemberExpression: null | |||
There was a problem hiding this comment.
We like to keep options as all the same data type, so null isn't appropriate for this.
There was a problem hiding this comment.
Would -1 be appropriate? EDIT: Whoops, ignore. You address this indirectly below. Just don't set it to anything at all. Got it.
There was a problem hiding this comment.
@nzakas why there is a null value for outerIIFEBody? Should we change that too in another PR?
The `MemberExpression` option should be opt-in as it is a breaking change.
|
LGTM |
|
Comment from @nzakas addressed (I think), commit squashed, branch force pushed. |
|
Lgtm, just waiting to let others review. |
|
LGTM, waiting for others to review. |
| @@ -1428,7 +1441,7 @@ ruleTester.run("indent", rule, { | |||
| { | |||
| code: fixture, | |||
| output: fixedFixture, | |||
| options: [2, {SwitchCase: 1}], | |||
| options: [2, {SwitchCase: 1, MemberExpression: 1}], | |||
There was a problem hiding this comment.
Why there is sometimes spaces after/before {/} and sometimes not?
This remark shouldn't block this PR
There was a problem hiding this comment.
This inconsistency seems to exist throughout the file.
For example, SwitchCase without a space happens on lines 266, 537, 542, and ten additional places.
But SwitchCase with a space occurs in lines 776 and 1897.
If the inconsistency is something that should be fixed eventually, maybe I can leave as is for this PR and I or someone else can open up a separate issue about this? Might be a good one to apply the beginner label to.
There was a problem hiding this comment.
The style guide seems to prefer object literals never being on one line in the first place, and so doesn't express an opinion about spaces after curly braces. I'll create an issue asking for clarification on that point. It might be that we can enable object-curly-spacing in this repository if we can reach consensus.
There was a problem hiding this comment.
hm, could one of you create an issue about it? I think it would be good if our code should be as consistent as possible so it could be kinda of an example how things should look like?
|
We usually need two "LGTM" right? LGTM |
eslint-deprecated
bot
added
the
archived due to age
What issue does this pull request address?
#6797
What changes did you make? (Give an overview)
Turn
MemberExpressionoption of theindentrule to be off by default.Is there anything you'd like reviewers to focus on?
The
MemberExpressionoption should be opt-in as it is a breakingchange.