Navigation Menu

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: preserve whitespace in multiline-comment-style (fixes #12312) #12316

Merged
merged 7 commits into from Nov 18, 2019

Conversation

kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Sep 26, 2019

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

fixes #12312

What changes did you make? (Give an overview)

This PR preserves leading whitespace when converting a block comment to a "starred" block comment in the multiline-comment-style rule's autofixer. Since this rule doesn't know about indentation, I decided to take the strategy of checking if the leading whitespace matches the expected whitespace (i.e. the whitespace before the opening delimiter for the block comment) and, if it does, to use any remaining whitespace after the asterisk. I think this will work for the vast majority of cases (though it could get weird if folks mix tabs and spaces). If they don't match, it falls back to the current behavior and adds one space.

Apologies for the large diff - I ended up doing a lot of refactoring to try to make the code easier to follow. All the calculation of how much whitespace should be used to offset each line now gets calculated in getCommentLines(), which I think is a clearer separation of concerns between this module's internal functions.

I believe the autofix should be consistent across formats now. In the case of an easy to use delimiter (* for starred-block or // for separate-lines), calculating the whitespace is pretty straightforward. For bare-block, the check now iterates through the lines of the comment and checks if any lines are indented less than the opening line, and offsets all the lines by the difference. If it's indented more, it removes the appropriate amount of whitespace. Otherwise, if it's correct, it returns the line unchanged.

Is there anything you'd like reviewers to focus on?

  1. This only preserves whitespace if the the leading whitespace on the line matches the leading whitespace before the opening delimiter of the block comment (/*). Does this seem like a good way to go about this? In other cases, I'm not sure how you would know exactly how much to indent something (particularly if there's a mix of tabs and spaces).
  2. Not directly related to this change, but I think it's a bit strange that we don't push a space after the asterisk when we convert line comments to "starred" block comments. I have added a test to document this behavior.
  3. I'm not sure how we would handle offsets in the case of code mixing tabs and spaces. If someone has an idea of how we could more gracefully check for this, I'm all ears!

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Sep 26, 2019
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Sep 26, 2019
@mysticatea
Copy link
Member

Thank you for fixing.

Would you add tests for other options? For example,

        {
            code: `
                /*
                 * {
                 *    "foo": 1,
                 *    "bar": 2
                 * }
                 */
            `,
            output: `
                // {
                //    "foo": 1,
                //    "bar": 2
                // }
            `,
            options: ["separate-lines"],
            errors: [
                { messageId: "expectedLines", line: 2 }
            ]
        }

@kaicataldo kaicataldo added the do not merge This pull request should not be merged yet label Oct 7, 2019
@kaicataldo
Copy link
Member Author

kaicataldo commented Oct 7, 2019

WIP! Updated this PR but added the "do not merge" label because I need to go back through and refactor/clean up the PR.

@kaicataldo kaicataldo force-pushed the fixes12312 branch 4 times, most recently from 4844b82 to 50b9391 Compare October 7, 2019 22:30
@kaicataldo
Copy link
Member Author

This is ready for re-review!

@kaicataldo kaicataldo removed the do not merge This pull request should not be merged yet label Oct 7, 2019
// {
// "foo": 1,
// "bar": 2
// }${" "}
Copy link
Member Author

Choose a reason for hiding this comment

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

We could update the autofixer to not include trailing whitespace, but I wasn't sure it was worth the extra complexity. Thoughts?

I added this because many people configure their editors to remove trailing whitespace automatically, and that could lead to a frustratingly vague error when running the tests.

@kaicataldo
Copy link
Member Author

@mysticatea This has been updated!

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Just one question. Thanks for working on this!

tests/lib/rules/multiline-comment-style.js Outdated Show resolved Hide resolved
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

Looks good!

I appreciate the explicit template interpolations for trailing whitespace as it makes it much easier to know, at a glance, what is going on.

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Looks really good to me, thank you!

But I have a concern that the autofix removes empty lines in some situations.

lib/rules/multiline-comment-style.js Outdated Show resolved Hide resolved
lib/rules/multiline-comment-style.js Outdated Show resolved Hide resolved
@kaicataldo
Copy link
Member Author

I have two last failing tests to fix. Marking as do not merge until I do so.

@kaicataldo kaicataldo added the do not merge This pull request should not be merged yet label Oct 25, 2019
@kaicataldo kaicataldo removed the do not merge This pull request should not be merged yet label Nov 7, 2019
@kaicataldo
Copy link
Member Author

Sorry for the delay! This ended up being a much more complex change than I anticipated 😄 Thanks for the great reviews!

@kaicataldo
Copy link
Member Author

@mysticatea Friendly ping :)

@mysticatea mysticatea self-requested a review November 11, 2019 08:24
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

Thank you for the update and sorry for my late response.

Looks really great to me!
There are nice refactoring and many tests.

@kaicataldo kaicataldo merged commit 62623f9 into master Nov 18, 2019
@kaicataldo kaicataldo deleted the fixes12312 branch November 18, 2019 17:22
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 18, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion autofix This change is related to ESLint's autofixing capabilities bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[multiline-comment-style] autofix removes indentation in the comment
3 participants