-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Fix quotes #1098
Fix quotes #1098
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1098 +/- ##
=======================================
Coverage 71.92% 71.92%
=======================================
Files 5 5
Lines 463 463
Branches 148 148
=======================================
Hits 333 333
Misses 130 130
Continue to review full report at Codecov.
|
@lbogdan OK so you stumbled across a false-positive here. Your change fixed the immediate issue, but your change doesn't address the actual issue at play here: Node 4 and Node 6 don't respect package-lock.json. Node 4/6 were installing eslint@4.7.0, which fails as you're seeing. Node 8 however, doesn't fail because it's respecting the package-lock.json versions, which is eslint@4.6.1. The work to fix this should address the changes in eslint between 4.7.0 and 4.6.1 in terms of config, before changing the code. Update: This bug has been reported to ESLint and they're aware of it eslint/eslint#9318. As stated in that issue, it should be cleared up with a patch release on Monday. Going to leave this PR open for the time being, but will likely close when ESLint issues that patch. |
@shellscape I knew this doesn't fix the root problem, but I didn't have time to investigate further. Even so, for the sake of consistency, I think those two |
@lbogdan I understand what you're saying, but I'm not a fan of band-aiding when a deeper issue exists. Another collaborator may disagree (and I'd be cool with that) but I can't approve and merge this until the greater issue with ESLint is resolved. Holding up a few PRs until they release a patch is OK by me. |
@shellscape I am completely with you on fixing the ESLint issue. I didn't say for a second to not fix it, I hope it didn't come through that way. Now, back to that block of code, I really don't see the reason for those two I mean, can't we fix both? |
@lbogdan yes, and I'm glad you arrived at that conclusion. to merge this (and to make this a full fix) the disable/enable directives should be removed and the other double quote on Line 131 should be cleaned up. As I practice I'll defer to you as to whether or not you'd like to add that to this PR (please do!). |
Thanks for getting that in. |
@shellscape Thanks! I also rebased my two other PRs. |
What kind of change does this PR introduce?
Quotes fix.
Did you add or update the
examples/
?Not needed.
Summary
Double quotes in
Server.js
made eslint fail (strangely enough, only in node 4 and 6).Does this PR introduce a breaking change?
No.