-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Fix] Add missing io.js installing message #1989
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
Conversation
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.
Thanks! It'd be nice to add a test for this tho, if possible.
a0adb76
to
a270ca0
Compare
Is it really necessary to squash commits here? They are individually meaningful and not the totally same thing. |
How is the test commit meaningful, separate from the fix? |
It prevents the regression of the bug coming back, isn't it :) |
Sure, I see that as the same act as the fix itself - iow, it's not actually a fix if it doesn't include the regression test. |
So I'd like to try to add them in the same PR, but the same commit? I just prefer to make them separately. |
The test commit doesn't look meaningless to me. Though I know the tests haven't passed yet 😅 |
bf95ce9
to
6872943
Compare
I'm still trying to fix that tests based on my local changes. So the commits will still be separately, if they are required to be squashed, maybe can do that once eveything is good. |
6872943
to
de38d2d
Compare
To make the case easier here, I'd like to just enable |
That seems totally reasonable! |
de38d2d
to
fc67426
Compare
@ljharb looks like |
This test isn't testing the "fetching" output, using |
fc67426
to
5170a8d
Compare
I didn't mean just this test though, also fine? |
The current wget output is like this:
|
Maybe we can |
I'm fine with that. |
f115565
to
6429302
Compare
I guess |
Fix #1988