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 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 eslint bot added the triage label Sep 26, 2019
@kaicataldo kaicataldo force-pushed the fixes12312 branch from 428cabc to c478675 Sep 26, 2019
@platinumazure platinumazure requested a review from mysticatea Sep 29, 2019
@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Sep 29, 2019

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

This comment has been minimized.

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 Oct 7, 2019
@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Oct 7, 2019

This is ready for re-review!

@kaicataldo kaicataldo removed the do not merge label Oct 7, 2019
// {
// "foo": 1,
// "bar": 2
// }${" "}

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Oct 7, 2019

Author Member

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 kaicataldo force-pushed the fixes12312 branch 3 times, most recently from 60b3456 to a77a6d4 Oct 7, 2019
@kaicataldo kaicataldo requested a review from platinumazure Oct 11, 2019
@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Oct 11, 2019

@mysticatea This has been updated!

Copy link
Member

platinumazure left a comment

Just one question. Thanks for working on this!

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

platinumazure left a comment

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 left a comment

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 kaicataldo force-pushed the fixes12312 branch from 71d6c76 to 0e7cb29 Oct 25, 2019
@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Oct 25, 2019

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

@kaicataldo kaicataldo force-pushed the fixes12312 branch from 1cec21f to 3c9007f Nov 7, 2019
@kaicataldo kaicataldo removed the do not merge label Nov 7, 2019
@kaicataldo kaicataldo force-pushed the fixes12312 branch from 3c9007f to f3f1232 Nov 7, 2019
@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Nov 7, 2019

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

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Nov 11, 2019

@mysticatea Friendly ping :)

@mysticatea mysticatea self-requested a review Nov 11, 2019
Copy link
Member

mysticatea left a comment

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
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191118.3 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@kaicataldo kaicataldo deleted the fixes12312 branch Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.