Fix indentation of closing parentheses #620
Conversation
|
This is a bug fix, I don't think it should be an option - I think if the test fixtures are wrong, they are what should be fixed. I'll let @yannickcr comment on the implementation. |
stefanbuck
force-pushed
the
fix-618
branch
2 times, most recently
from
af8bb41
to
d9e6c6f
Compare
|
I see your point, but I'm worried that too many tests would fail, because of this change. Just my two cents. The remaining failing tests are fixed with c65bfd7 |
|
That just means that too many tests were wrong before. When the changes are just "how many errors" and not "it wasn't an error and now it is", that's not consequential. |
|
@yannickcr what do you think about the implementation? |
|
I'm still interested in finishing this PR @yannickcr @ljharb |
tests/lib/rules/jsx-indent.js
Outdated
| errors: [{message: 'Expected indentation of 2 space characters but found 9.'}] | ||
| errors: [ | ||
| {message: 'Expected indentation of 2 space characters but found 9.'}, | ||
| {message: 'Expected indentation of 2 space characters but found 9.'} |
There was a problem hiding this comment.
Why do you have 2 errors here? We should only have one in this case (for the closing tag).
There was a problem hiding this comment.
You're right, it should be only one error. The openingIndent and closingIndent values are correct, but I think the report() is called with the wrong node. I tried to pass the openingElement and the closeingElement but still it's not working. Do you have anything in mind how to solve this?
|
@stefanbuck sorry for the delay :( |
|
@stefanbuck I've rebased your PR branch, but there's still some test failures, and I'm unable to push to it. Can you check the "allow edits" checkbox on the right hand side of the PR? After that, you can check out my changes and iterate from there. |
|
@ljharb try it again, please. |
|
@stefanbuck thanks; updated. please delete your local branch, fetch, and re-check out the latest changes, and let's see if we can get this ready to land :-) |
|
@stefanbuck i've rebased this, but tests are failing. can you take another look? |
|
Nothing is incorrect here; commit dates are irrelevant. I already rebased the branch for you - if you delete it and check it out fresh, it should work fine. |
tests/lib/rules/jsx-indent.js
Outdated
| errors: [ | ||
| {message: 'Expected indentation of 2 space characters but found 9.'}, | ||
| {message: 'Expected indentation of 2 space characters but found 9.'} | ||
| ] |
There was a problem hiding this comment.
Tests were failing with
AssertionError [ERR_ASSERTION]: Should have 1 error but had 2:
[
{
ruleId: 'jsx-indent',
severity: 1,
message: 'Expected indentation of 2 space characters but found 9.',
line: 2,
column: 3,
nodeType: 'ReturnStatement',
endLine: 4,
endColumn: 17,
fix: {range: [Array], text: ' '}
},
{
ruleId: 'jsx-indent',
severity: 1,
message: 'Expected indentation of 2 space characters but found 9.',
line: 4,
column: 10,
nodeType: 'JSXClosingElement',
endLine: 4,
endColumn: 16,
fix: {range: [Array], text: ' '}
}
];
+ expected - actual
-2
+1
Looks like #1956 added handling for a few similar cases like I did.
|
@ljharb failing tests are fixed |
|
@stefanbuck I've rebased this again adding |
ljharb
force-pushed
the
fix-618
branch
2 times, most recently
from
bba0015
to
85823ee
Compare
Fixes jsx-eslint#618. Co-authored-by: Stefan Buck <[email protected]> Co-authored-by: Jordan Harband <[email protected]>
|
Rebased, and added the fixer changes needed to make the tests pass. |
Codecov Report
@@ Coverage Diff @@
## master #620 +/- ##
=======================================
Coverage 97.55% 97.55%
=======================================
Files 121 121
Lines 8368 8384 +16
Branches 3012 3016 +4
=======================================
+ Hits 8163 8179 +16
Misses 205 205
Continue to review full report at Codecov.
|
This PR will fix the issue describe in #618 where the indentation of the closing parentheses on return statements was not working.
I know some tests are failing, because the existing fixtures doesn't fit with the new behaviour. Therefore I think this should be enabled with an option. Before I fix that, I would like to get some feedback if the implementation is good so far.
Fixes #618.