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 quotes #1098

Merged
merged 2 commits into from Sep 18, 2017
Merged

Fix quotes #1098

merged 2 commits into from Sep 18, 2017

Conversation

lbogdan
Copy link
Contributor

@lbogdan lbogdan commented Sep 16, 2017

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.

@codecov
Copy link

codecov bot commented Sep 16, 2017

Codecov Report

Merging #1098 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1098   +/-   ##
=======================================
  Coverage   71.92%   71.92%           
=======================================
  Files           5        5           
  Lines         463      463           
  Branches      148      148           
=======================================
  Hits          333      333           
  Misses        130      130
Impacted Files Coverage Δ
lib/Server.js 78.59% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc22935...aa42892. Read the comment docs.

@lbogdan
Copy link
Contributor Author

lbogdan commented Sep 16, 2017

Please merge so I can rebase #1096 and #1097, whose tests are currently failing because of this. Thanks!

@shellscape
Copy link
Contributor

@lbogdan this is really strange, as neither Travis nor my local install fail on this. the quotes rule is disabled for that block at line 110 and enabled on line 144

@shellscape
Copy link
Contributor

shellscape commented Sep 17, 2017

@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.

@lbogdan
Copy link
Contributor Author

lbogdan commented Sep 17, 2017

@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 eslint-disable/enable directives can be removed and the quotes replaced, like in this PR.

@shellscape
Copy link
Contributor

@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.

@lbogdan
Copy link
Contributor Author

lbogdan commented Sep 17, 2017

@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 eslint-disable/enable directives and using single and double quotes interchangeably.

I mean, can't we fix both?

@shellscape
Copy link
Contributor

@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!).

@shellscape shellscape merged commit 3e24ac4 into webpack:master Sep 18, 2017
@shellscape
Copy link
Contributor

Thanks for getting that in.

@lbogdan lbogdan deleted the fix/quotes branch September 18, 2017 15:48
@lbogdan
Copy link
Contributor Author

lbogdan commented Sep 18, 2017

@shellscape Thanks! I also rebased my two other PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants