search

mbrevoort / node-reggie

An experimental light weight alternative to a full blown npm registry
416 stars 50 forks source link

Implemented possibility to configure the host. #32

Closed akoenig closed 11 years ago

akoenig commented 11 years ago

Closes #31

bajtos commented 11 years ago

Hello @akoenig, thank you for your pull request. The change looks generally good, there are two things to improve:

  1. Please rename the option, AFAIK most applications use --host to configure the host:

    -h --host  Which host should Reggie listen on? [default: '0.0.0.0']
  2. See the inline comment above.
akoenig commented 11 years ago

Sure, I will do that.

I also thought about "host" for this parameter. Unfortunately the "h" collides with the help command, so what do you think? No alias at all?

Edit: Ha, just saw your comment in line https://github.com/mbrevoort/node-reggie/blob/master/server.js#L18 :) So, yes, here you go :)

bajtos commented 11 years ago

I also thought about "host" for this parameter. Unfortunately the "h" collides with the help command, so what do you think? No alias at all?

IMO --help is the canonical way for getting help. It's quite common to use -h for something else, see e.g. GNU grep:

-h, --no-filename Suppress the prefixing of filenames on output when multiple files are searched.

I think it's ok to change -h from --help to --host.

bajtos commented 11 years ago

Almost good to merge - see the comment above.

BTW, sorry for reacting so late. Github unfortunately does not send a notification when a commit was added to a pull request. Therefore it's better to write an explicit comment, so that maintainers are notified about your update.

akoenig commented 11 years ago

Ah, sorry about that. Done :)

bajtos commented 11 years ago

Landed as 8f63bd0, released in v0.2.0. Thank you for your contribution.