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

Fix CLI host parameter short name #157

Merged

Conversation

minupalaniappan
Copy link
Contributor

@minupalaniappan minupalaniappan commented Jun 6, 2023

Originally, I was not able to run the JSON GraphQL server at port 5000 with -p 5000.

On this branch, you can now run the the command ./bin/json-graphql-server db.js -p 5000 or ./bin/json-graphql-server db.js --port 5000

This PR fixes an inconsistency between the --host and --port parameters of the CLI, which short names should be -h and -p respectively. This is also what's documented in the Readme.

Copy link
Collaborator

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.
However I don't understand the change, which seems to do the opposite of what you describe.
Currently, according to my tests, the script works fine both with --port 5000 and -p 5000.

Now it might be argued that the issue is rather with the host parameter, which should be -h rather than --h, to be consistent with the other parameter and with the docs.

This is what I suggested in this review.

bin/json-graphql-server.js Outdated Show resolved Hide resolved
bin/json-graphql-server.js Outdated Show resolved Hide resolved
@slax57 slax57 changed the title Enable --host and --port as CLI params for custom host and port. READ… Fix CLI parameters short name Jun 7, 2023
minupalaniappan and others added 2 commits June 7, 2023 10:42
Co-authored-by: Jean-Baptiste Kaiser <jbaptiste.kaiser@gmail.com>
Co-authored-by: Jean-Baptiste Kaiser <jbaptiste.kaiser@gmail.com>
@minupalaniappan
Copy link
Contributor Author

Whoops, apologies for the bug in the -- for p and h. Your suggestions look fantastic, and I've updated the PR per your comment. Thank you 🤌🏽

@slax57 slax57 added this to the 2.3.3 milestone Jun 8, 2023
Copy link
Collaborator

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Thanks

@slax57 slax57 merged commit a2c2858 into marmelab:master Jun 8, 2023
@slax57 slax57 changed the title Fix CLI parameters short name Fix CLI host parameter short name Jun 8, 2023
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.

None yet

2 participants