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

Implement basic RESTful server #12

Merged
merged 1 commit into from Feb 8, 2016

Conversation

timofurrer
Copy link
Contributor

No description provided.

@ncrocfer
Copy link
Owner

ncrocfer commented Feb 8, 2016

It's your turn to have conflict with @brouberol ^^ Can you fix it in __main__.py please ?

++<<<<<<< HEAD
 +from .core import get_ports, get_dict
++=======
+ from .core import get_ports
+ from .server import app
++>>>>>>> 263b7ab6806e88de6b1384c5d2ff4c22bbf90a19
...
++<<<<<<< HEAD
 +@click.option('--json', is_flag=True, default=False,
 +              help='Format the output result as JSON.')
 +def run(port, like, json):
++=======
+ @click.option("--server", is_flag=True, default=False,
+               help="Start a RESTful server.")
+ @click.option("--server-host", default="localhost",
+               help="Hostname for the server.")
+ @click.option("--server-port", default=8080,
+               help="Port for the server.")
+ def run(port, like, server, server_host, server_port):
++>>>>>>> 263b7ab6806e88de6b1384c5d2ff4c22bbf90a19

By the way it's a very good and useful (I think) PR, but I have 2 comments :

  • originally the tool was designed to be simple, now we have many options for a single feature, (--server, --server-host, --server-port). Can you merge it in 1 single option to have a cleaner help ?
    Example : whatportis --server 127.0.0.1 8000 instead of whatportis --server --server-host=127.0.0.1 --server-port=8000. It's very simple with click (http://click.pocoo.org/5/options/#multi-value-options).
  • and can you update the README to inform users about this new feature please ? For example telling them how to launch the server, and 1 or 2 curl examples (curl 127.0.0.1:8080/ports/5432, curl 127.0.0.1:8080/ports/mysql?like).

Thank you :)

@timofurrer
Copy link
Contributor Author

It's updated now ;)

@ncrocfer
Copy link
Owner

ncrocfer commented Feb 8, 2016

Arff, apparently click does not support default values for narg > 1 (pallets/click#472 (comment)) !
I merge it anyway, thanks 👍

ncrocfer added a commit that referenced this pull request Feb 8, 2016
@ncrocfer ncrocfer merged commit 36dbde5 into ncrocfer:master Feb 8, 2016
@timofurrer
Copy link
Contributor Author

I know - that's why I implemented it the way I did ;) 🍻

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