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] Add missing io.js installing message #1989

Merged
merged 1 commit into from
Jan 23, 2019

Conversation

PeterDaveHello
Copy link
Collaborator

Fix #1988

@ljharb ljharb added installing node Issues with installing node/io.js versions. bugs Oh no, something's broken :-( io.js This relates to https://iojs.org/ labels Jan 22, 2019
@ljharb ljharb changed the title [Fix] Add missing io.js installaing message [Fix] Add missing io.js installing message Jan 22, 2019
Copy link
Member

@ljharb ljharb left a 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.

@PeterDaveHello
Copy link
Collaborator Author

Is it really necessary to squash commits here? They are individually meaningful and not the totally same thing.

@ljharb
Copy link
Member

ljharb commented Jan 22, 2019

How is the test commit meaningful, separate from the fix?

@PeterDaveHello
Copy link
Collaborator Author

It prevents the regression of the bug coming back, isn't it :)

@ljharb
Copy link
Member

ljharb commented Jan 22, 2019

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.

@PeterDaveHello
Copy link
Collaborator Author

So I'd like to try to add them in the same PR, but the same commit? I just prefer to make them separately.

@PeterDaveHello
Copy link
Collaborator Author

The test commit doesn't look meaningless to me.

Though I know the tests haven't passed yet 😅

@PeterDaveHello
Copy link
Collaborator Author

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.

@PeterDaveHello
Copy link
Collaborator Author

To make the case easier here, I'd like to just enable --no-progress for nvm install ... So I don't need to handle the progress bar which already has its own unit test.

@ljharb
Copy link
Member

ljharb commented Jan 22, 2019

That seems totally reasonable!

@PeterDaveHello
Copy link
Collaborator Author

@ljharb looks like wget was not properly tested before, and its current output will contain timestamp, which makes the expected output in the tests harder to mock, can we just enable --quiet to disable its output?

@ljharb
Copy link
Member

ljharb commented Jan 23, 2019

This test isn't testing the "fetching" output, using --quiet seems totally fine.

@PeterDaveHello
Copy link
Collaborator Author

I didn't mean just this test though, also fine?

@PeterDaveHello
Copy link
Collaborator Author

The current wget output is like this:

$ wget -c https://iojs.org/dist/v1.0.0/iojs-v1.0.0-linux-x64.tar.xz -nv
2019-01-23 15:47:16 URL:https://iojs.org/dist/v1.0.0/iojs-v1.0.0-linux-x64.tar.xz [5325772/5325772] -> "iojs-v1.0.0-linux-x64.tar.xz" [1]

@ljharb
Copy link
Member

ljharb commented Jan 23, 2019

Maybe we can head 1 it? I'm only interested in testing Downloading and installing io.js v1.0.0...

@PeterDaveHello
Copy link
Collaborator Author

I'm fine with that.

@PeterDaveHello
Copy link
Collaborator Author

I guess head will cut off the progress of the installation, so will use a trick to wait for the whole process properly finished.

@ljharb ljharb merged commit cc0750e into nvm-sh:master Jan 23, 2019
@PeterDaveHello PeterDaveHello deleted the fix-#1988 branch January 24, 2019 06:03
@NKjoep NKjoep mentioned this pull request Nov 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugs Oh no, something's broken :-( installing node Issues with installing node/io.js versions. io.js This relates to https://iojs.org/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants