-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Chore: Clean up inline directive parsing #12375
Conversation
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I agree that this is easier to grok. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
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? |
@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.
Thanks @platinumazure. Updated and new review requested. |
LGTM, thanks for contributing! |
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:
directiveType
out of the comment.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:
if(lineCommentSupported)
nor theelse if (comment.type === "Block")
case and thus fall through without ever getting adirectiveType
assigned.directiveType
foreslint-disable-next-line
andeslint-disable-line
the same way we handle all other directives.The content of all pre-existing case statements have not actually changed.