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: Extract config comment parsing #11091
Conversation
Hmm, looks like the build is breaking only in Node 11 due to a completely unrelated problem. MockFS |
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, thanks for doing this! (It's been on my list for quite some time...)
There are a lot of commented tests. What's the plan for those?
lib/util/config-comment-parser.js
Outdated
constructor() { | ||
debug("Created ConfigCommentParser"); | ||
|
||
// empty for now |
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.
Are we planning to put more in here? Seems like it's pretty clear to me this is only used for debugging purposes at the moment.
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.
I was thinking at some point I'd add a strict
option to remove some of the fallback behavior we have. Right now it's just a placeholder though. I can remove it if it's vexing.
@platinumazure what commented tests are you referring to? |
e6da160
to
2fea543
Compare
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, thanks!
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:
Extracted config comment parsing into a separate utility.
What changes did you make? (Give an overview)
While digging around in
Linter
, I realized that the config comment parsing could be completely separated out, allowing us to better test config comment parsing. I just extracted the existing helper functions fromlinter.js
and put them into a class. I also wrote up some basic tests based on the comments inside the code to verify everything works. At some point I'd like to go back and clean up the methods a bit to make them less tied to what's going on inside oflinter.js
, but for now this helps makelinter.js
smaller and gives us a place to test for further changes in comment parsing.I'd really like to make
linter.js
smaller as it's over 1,000 lines right now and a bit hard to follow.Is there anything you'd like reviewers to focus on?
Does this approach make sense?