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.

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

Fix indentation of closing parentheses #620

Merged
merged 2 commits into from Feb 2, 2022
Merged

Conversation

stefanbuck
Copy link
Contributor

@stefanbuck stefanbuck commented Jun 3, 2016

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.

@ljharb
Copy link
Member

ljharb commented Jun 3, 2016

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 stefanbuck force-pushed the fix-618 branch 2 times, most recently from af8bb41 to d9e6c6f Compare June 4, 2016 06:03
@stefanbuck
Copy link
Contributor Author

stefanbuck commented Jun 4, 2016

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

@coveralls
Copy link

coveralls commented Jun 4, 2016

Coverage Status

Coverage increased (+0.01%) to 96.779% when pulling c65bfd7 on stefanbuck:fix-618 into 82b3aa9 on yannickcr:master.

@ljharb
Copy link
Member

ljharb commented Jun 4, 2016

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.

@stefanbuck
Copy link
Contributor Author

@yannickcr what do you think about the implementation?

@stefanbuck
Copy link
Contributor Author

I'm still interested in finishing this PR @yannickcr @ljharb

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.'}
Copy link
Member

Choose a reason for hiding this comment

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

Why do you have 2 errors here? We should only have one in this case (for the closing tag).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

@yannickcr
Copy link
Member

@stefanbuck sorry for the delay :(

@ljharb
Copy link
Member

ljharb commented Dec 27, 2018

@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.

@stefanbuck
Copy link
Contributor Author

@ljharb try it again, please.

@ljharb
Copy link
Member

ljharb commented Dec 28, 2018

@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 :-)

@ljharb
Copy link
Member

ljharb commented Dec 13, 2019

@stefanbuck i've rebased this, but tests are failing. can you take another look?

@stefanbuck
Copy link
Contributor Author

@ljharb I'm unable to rebase because of #2525 which is super weird.

@ljharb
Copy link
Member

ljharb commented Dec 15, 2019

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.

errors: [
{message: 'Expected indentation of 2 space characters but found 9.'},
{message: 'Expected indentation of 2 space characters but found 9.'}
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@stefanbuck
Copy link
Contributor Author

@ljharb failing tests are fixed

@ljharb
Copy link
Member

ljharb commented Dec 27, 2019

@stefanbuck I've rebased this again adding output to the invalid test cases, to test the autofixer, and the tests are failing.

ljharb and others added 2 commits February 2, 2022 13:27
Fixes jsx-eslint#618.

Co-authored-by: Stefan Buck <stefanb@brandwatch.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@ljharb
Copy link
Member

ljharb commented Feb 2, 2022

Rebased, and added the fixer changes needed to make the tests pass.

@ljharb ljharb marked this pull request as ready for review February 2, 2022 21:53
@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2022

Codecov Report

Merging #620 (c9b7294) into master (f8b4aa3) will increase coverage by 0.00%.
The diff coverage is 94.73%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
lib/rules/jsx-indent.js 97.61% <94.73%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8b4aa3...c9b7294. Read the comment docs.

@ljharb ljharb merged commit c9b7294 into jsx-eslint:master Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

react/jsx-indent: Closing parenthese indentation
5 participants