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

Allow location of settings.json to be overridden through cli arg #455

Closed
wants to merge 3 commits into from
Closed

Allow location of settings.json to be overridden through cli arg #455

wants to merge 3 commits into from

Conversation

jhollinger
Copy link
Contributor

Resolves issue #195, allowing the location of the settings file to passed as a parameter. The following should all work:

bin/run.sh --settings settings2.json
bin/run.sh --settings /abs/path/to/settings.json
node server.js --settings my_settings.json
...

Unfortunately this adds a dependency (optimist) that converts cli args into a hash object. However that should make adding other options quite simple in the future.

@Pita
Copy link
Contributor

Pita commented Feb 16, 2012

good idea, we just need to document that somewhere so we don't forget about this feature

@fourplusone
Copy link
Contributor

I think a --help / -h flag would be a good place for documenting startup parameters

@Pita
Copy link
Contributor

Pita commented Feb 16, 2012

@fourplusone thats right if you have lots of command line parameters, but so far we have only one, and this one is optional

@jhollinger
Copy link
Contributor Author

Even though there's currently only one option, IMHO -h/--help is a sensible place to "document" it. People expect to find things that way. And I bet more will be added over time.

@fourplusone
Copy link
Contributor

I would suggest to handle the command line parameters in its own module. See: fourplusone@fa15d7a

@jhollinger
Copy link
Contributor Author

Almost seems overkill at this point, but I guess it is a little cleaner, especially if we add more options.

If we do add a -h/--help option, bin/run.sh should detect and send it to server.js immediately, and never call bin/installDeps.sh, etc.

@fourplusone
Copy link
Contributor

If there are missing deps skipping installDeps.sh will cause a crash, even if the user only wants to read --help

@jhollinger
Copy link
Contributor Author

IMO waiting on installDeps, just to see -h/--help is super annoying. It can take awhile. And it prints out lots of junk.

@fourplusone You're right about missing deps = crash, but we can get around that. The simplest way would be to keep your Cli module, but strip out optimist. (It's not too difficult to manually parse process.argv.slice(2)). Then if that module is loaded first, since it has no deps, it can print --help and exit.

@jhollinger
Copy link
Contributor Author

This latest commit incorporates some of @fourplusone's ideas, but removes optimist, so that -h/--help can be run before any dependencies are ever installed.

If the idea is accepted, the commit history probably needs to be cleaned up.

@Pita
Copy link
Contributor

Pita commented Feb 17, 2012

A wiki article called "how to pass the settings file via command line" would have been enough imho

@jhollinger
Copy link
Contributor Author

@Pita should I revert the -h/--help changes? I was curious how it would play out. But with only one option, I could go either way.

@jhollinger
Copy link
Contributor Author

I am closing this in light of pull request #478.

@jhollinger jhollinger closed this Feb 21, 2012
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.

3 participants