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

Chore: Clean up inline directive parsing #12375

Merged
merged 3 commits into from Oct 8, 2019

Conversation

@captbaritone
Copy link
Contributor

@captbaritone captbaritone commented Oct 4, 2019

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

[ ] Documentation update
[ ] 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
[X] Other, please explain:

Clean up somewhat confusing code.

What changes did you make? (Give an overview)

While exploring what would be required to implement RFC #34 I looked into this code and found it more confusing than it needs to be.

This refactor untagles some logic having to do with parsing inline directives

There are a few thing going on in this function which were getting conflated:

  1. Parsing a directiveType out of the comment.
  2. Ignoring directives that are in line comments but only support block comments.
  3. Warning on and ignoring line comments that span multiple lines.

Previously these three pieces of functionality were tightly coupled which made the code harder to read. After this change each task is handled independently of the other.

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

GitHub makes this diff look more significant than it really is. The change actually consists of a few small changes:

  1. Explicitly ignore line comments for directives that don't support line comments. Previously this was done implicitly because these instances would pass neither the if(lineCommentSupported) nor the else if (comment.type === "Block") case and thus fall through without ever getting a
    directiveType assigned.
  2. Derive a directiveType for eslint-disable-next-line and eslint-disable-line the same way we handle all other directives.

The content of all pre-existing case statements have not actually changed.

captbaritone added 2 commits Oct 4, 2019
This simply makes the code a bit easier to read by giving a name to `match[1]`.
There are a few thing going on in this function which were getting
conflated:

1. Parsing a `directiveType` out of the comment.
2. Ignoring directives that are in line comments but only support block
   comments.
3. Warning on and ignoring line comments that span multiple lines.

Previously these three pieces of functionality were tightly coupled
which made the code harder to read. After this change each task is
handled independently of the other.
@eslint-deprecated eslint-deprecated bot added the triage label Oct 4, 2019
@captbaritone captbaritone changed the title Clean up inline directive parsing Chore: Clean up inline directive parsing Oct 4, 2019
@platinumazure platinumazure added chore core and removed triage labels Oct 4, 2019
Copy link
Member

@kaicataldo kaicataldo left a comment

LGTM. I agree that this is easier to grok. Thanks!

Copy link
Member

@platinumazure platinumazure left a comment

Looks good, thanks!

@captbaritone
Copy link
Contributor Author

@captbaritone captbaritone commented Oct 5, 2019

I just noticed a way this code could be further cleaned up. I have a commit on another branch here: 6231a5e

Given that this PR is already approved, would it be better to update this one or wait for this to merge and open a second one?

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Oct 5, 2019

@captbaritone I like what you're doing in the latest commit, so personally I would say you could pull that commit into this branch if you like.

If you want to make sure that the team still thinks it's good, you could re-request review from the team members who already approved.

That said, it's up to you re: how you want to proceed.

Rather than conditionally set a mutable value and check for it at the end of the switch statement, we can actually just handle it inline by using a fallthrough.
@captbaritone captbaritone requested review from platinumazure and kaicataldo Oct 5, 2019
@captbaritone
Copy link
Contributor Author

@captbaritone captbaritone commented Oct 5, 2019

Thanks @platinumazure. Updated and new review requested.

@kaicataldo kaicataldo merged commit 7ffb22f into eslint:master Oct 8, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
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 #20191005.6 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
Copy link
Member

@kaicataldo kaicataldo commented Oct 8, 2019

LGTM, thanks for contributing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants