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

Remove 'v' shortcut for version #1222

Merged
merged 1 commit into from
Apr 24, 2015
Merged

Remove 'v' shortcut for version #1222

merged 1 commit into from
Apr 24, 2015

Conversation

zertosh
Copy link
Member

@zertosh zertosh commented Apr 19, 2015

This is part of an effort to bring consistency to the browserify and watchify flags (as somewhat discussed in browserify/watchify#204 (comment)).

watchify already uses "v" for "verbose", and has no "version" flag. By removing the (arguably) seldom used "v" from browserify, (1) watchify can fully "own" it, and (2) can add a "version" flag without clashing with browserify's short form of it. Also, a lot of unix tools don't have a "v", just "version".

Alternatively, "v" could be replaced with "V" - ssh, less, among other do this.

cc: @jmm

@terinjokes
Copy link
Contributor

What's the use case of having watchify be verbose without browserify also being verbose? From my perspective, I'd want my entire toolchain to be verbose while debugging, not just a portion of it.

@zertosh
Copy link
Member Author

zertosh commented Apr 20, 2015

@terinjokes Seems weird to have browserify be verbose when its output goes to stdout - but sure, maybe verbose logging should go to stderr?

FYI: watchify's verbose is simply "ZZZ bytes written to YYY.js (X seconds)" every time you rebuild. It's not really debugging. It's more like informational since it's a long running process.

@zertosh
Copy link
Member Author

zertosh commented Apr 21, 2015

@terinjokes @jmm Can we merge this in?

@jmm
Copy link
Collaborator

jmm commented Apr 21, 2015

@zertosh Sorry for delayed reply. I have no problem with it personally, but as I mentioned on another issue, I'm no authority on the CLI. So I think someone who has more interest / expertise with it should make the call.

@terinjokes
Copy link
Contributor

What do you mean by " watchify can fully "own" it"?

@zertosh
Copy link
Member Author

zertosh commented Apr 21, 2015

@terinjokes I mean that v only does what watchify already does with it.

Right now, the flag matrix looks like this:

flag browserify watchify
version version noop
v version verbose
verbose noop verbose

I think it makes more sense like this:

flag browserify watchify
version version version
v noop verbose
verbose noop verbose

If browserify were to implement some kind of "verbose", it would then look like this:

flag browserify watchify
version version version
v verbose verbose
verbose verbose verbose

@zertosh
Copy link
Member Author

zertosh commented Apr 21, 2015

(wow charts look pretty cool)

@zertosh
Copy link
Member Author

zertosh commented Apr 24, 2015

@terinjokes I'd really appreciate an answer on this. A version flag might seem trivial for watchify, but I get a lot of issues like browserify/watchify#190 (comment) - where users don't know whether they're using a local/global version and what version of browserify their watchify is using. The version flag I want to implement in watchify would answer those two questions (see browserify/watchify#197 (comment)).

Without the change, if I add the version flag to watchify, we'd be looking at:

flag browserify watchify
version version version
v version verbose
verbose noop verbose

Maybe I'm over thinking this. If you agree, then let me know so I can proceed.

terinjokes added a commit that referenced this pull request Apr 24, 2015
@terinjokes terinjokes merged commit 96d1662 into browserify:master Apr 24, 2015
@terinjokes
Copy link
Contributor

Merged, sorry for the delay.

@zertosh
Copy link
Member Author

zertosh commented Apr 24, 2015

@terinjokes No worries! It's cool. Thanks!

@zertosh zertosh mentioned this pull request Apr 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants