Skip to content

Proper CLI with commander #603

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

Merged
merged 7 commits into from
Feb 25, 2016
Merged

Proper CLI with commander #603

merged 7 commits into from
Feb 25, 2016

Conversation

flovilmart
Copy link
Contributor

Fully rewritten CLI with proper environment handling, and args parsing and configuration JSON.

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

1 similar comment
@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart changed the title Proper cli Proper CLI with commander Feb 23, 2016
@flovilmart
Copy link
Contributor Author

That will prevent anyone to fill in data into bin/parse-server!

@flovilmart flovilmart closed this Feb 23, 2016
@flovilmart flovilmart reopened this Feb 23, 2016
@flovilmart
Copy link
Contributor Author

Weird error on travis:

$ nvm install 4.3
Version '4.3' not found - try `nvm ls-remote` to browse available versions.
Remote repository may not be reachable
0.12s$ nvm use 4.3
N/A version is not installed yet

@flovilmart flovilmart closed this Feb 23, 2016
@flovilmart flovilmart reopened this Feb 23, 2016
@flovilmart flovilmart closed this Feb 23, 2016
@flovilmart flovilmart reopened this Feb 23, 2016
@flovilmart
Copy link
Contributor Author

@drew-gross if you have a minute for that, that will prevent all mis-usage of bin/parse-server that is piling up bugs...

},
"mountPath": {
env: "PARSE_SERVER_MOUNT_PATH",
help: "Mount path for the server, defaults to /"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest defaulting to /parse for consistency with parse-server-example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np!

@drew-gross
Copy link
Contributor

Looks cool. Are you thinking this should replace the current default method of running the server? Or add a new method? Either way I would like to see some updates to README.md and/or CONTRIBUTING.md before merging this.

@flovilmart
Copy link
Contributor Author

We could update the README.md definitely, npm start -- path/to/config.json is a must have IMHO or when installed globally parse-server path/to/config.json

@flovilmart
Copy link
Contributor Author

There you go!

it doesn't add a new method, bin/parse-server was there for a while. It just makes it harder to edit the bin/parse-server file that some users were doing.

This is my preferred method to run it, but that's personal.

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

var api = new ParseServer(options);
app.use(options.mountPath, api);

var port = process.env.PORT || 1337;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put port into the commander options too? Seems inconsistent to have just this one config be environment variable only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can!

@nlutsenko
Copy link
Contributor

Also, can we please use ES6 style for this one? It's a lot of nice and great new code, but not using ES6 for it looks pretty sad :(

@nlutsenko
Copy link
Contributor

Overall - looks like an amazing replacement for existing bin/parse-server! Great job!

@flovilmart
Copy link
Contributor Author

I'll ES6ify it!

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@flovilmart flovilmart closed this Feb 24, 2016
@flovilmart flovilmart reopened this Feb 24, 2016

`$ npm start -- path/to/your/config.json`

The default port is 1337, to use a different port set the PORT environment variable:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you updated the code for this setting but you forgot to update the docs :p

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idiot me, actually that still works :)

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@drew-gross
Copy link
Contributor

Sweet, I will merge this now so it has some time to stabilize before the next version. The fact that these docs are in master's README.md and will show up on the project front page despite not being available if you aren't running master kinda sucks, it makes me think it might be time to change our branching strategy.

drew-gross added a commit that referenced this pull request Feb 25, 2016
@drew-gross drew-gross merged commit a7262da into parse-community:master Feb 25, 2016
@flovilmart
Copy link
Contributor Author

@drew-gross we could have develop the working branch and default, master being the latest release. with tags?

@drew-gross
Copy link
Contributor

Seems reasonable to me. Let see what @nlutsenko and @gfosco think.

@nlutsenko
Copy link
Contributor

Let's stick with the default flow of GitHub - master is the default branch, which we never break and commit all things to it, meaning that we should be comfortable releasing master at any given point in time. This is the preferred flow for every single open source project. We can simply stage README chagnes that are set for next release in pull requests and merge them before the release, sounds good?

@gfosco
Copy link
Contributor

gfosco commented Feb 26, 2016

Agree with Nikita here, about keeping master default and staging readme changes... In addition I'd say we should release new versions whenever we have something, 1-2x per week...

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.

5 participants