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

Add --verbose flag #41

Closed
wants to merge 8 commits into from
Closed

Add --verbose flag #41

wants to merge 8 commits into from

Conversation

itaisteinherz
Copy link

@itaisteinherz itaisteinherz commented Dec 17, 2019

Resolves #38 (and will close #40).

Screenshot

@itaisteinherz
Copy link
Author

I just noticed #40 😅 (I worked on this for a few days now and only got to push it now). Sorry for not claiming the issue earlier on. @sindresorhus let me know which PR you'd prefer to go with.

cli.js Outdated Show resolved Hide resolved
@itaisteinherz
Copy link
Author

@sindresorhus Friendly ping 🏓

@sindresorhus
Copy link
Owner

Sorry, I was waiting for #40 as it was submitted first, but no response there, so it's all yours. I prefer the look of #40 though, so if you could make it look like that and address the feedback given there, it would be great.

@sindresorhus sindresorhus mentioned this pull request Apr 5, 2020
@sindresorhus sindresorhus changed the title Create verbose option Add verbose option Apr 5, 2020
@sindresorhus
Copy link
Owner

@itaisteinherz Bump :)

@Telematica
Copy link

Any news on this feature? Would love having it!

@sindresorhus
Copy link
Owner

@itaisteinherz Last bump

@itaisteinherz
Copy link
Author

@sindresorhus Thanks for pinging me, I'll get to it this evening and push my fixes.

@itaisteinherz
Copy link
Author

@sindresorhus I fixed the comments you left here.

cli.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Add verbose option Add --verbose flag Aug 29, 2020
cli.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Latency should be shown when it's ready. Not just at the end.

@sindresorhus
Copy link
Owner

Can you fix the merge conflict? (You also need to make --verbose compatible with the --single-line flag. It doesn't have to be a single line, just remove all unneeded whitespace when --single-line.

@kirubakaran
Copy link

@itaisteinherz thanks for your work. Hoping to see this in the master soon!

cli.js Show resolved Hide resolved
@itaisteinherz
Copy link
Author

@sindresorhus I applied your suggestions, resolved the conflicts and added support for using --verbose and --single-line together.

@sindresorhus
Copy link
Owner

There should not be an empty line between the verbose stuff:

     Latency:  10ms (unloaded)  112ms (loaded)

      Client:  Fredrikstad, NO 212.251.200.182 Telenor
     Servers:  Open Connect, Netflix

@sindresorhus
Copy link
Owner

Notice the unalignment here:

❯ node cli.js --verbose --single-line
100 Mbps ↓ / 28 Mbps ↑
Latency:  12ms (unloaded)  61ms (loaded)
Client:  Fredrikstad, NO 212.251.200.182 Telenor
Servers:  Open Connect, Netflix

@sindresorhus
Copy link
Owner

I think there should be one more empty line between the speed and Latency:.

@skywinder
Copy link

Nice update. looking forward to merge ❤️

skywinder added a commit to skywinder/fast-cli that referenced this pull request Sep 27, 2020
Base automatically changed from master to main January 23, 2021 11:09
@sindresorhus
Copy link
Owner

Closing in favor of #52

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.

show more details in results
5 participants