Added shutting down calls to the visualization server #241
Conversation
|
@pydy/pydy-developers This needs a review. Should be a quick one. |
| @@ -1,37 +1,107 @@ | |||
| #!/usr/bin/env python | |||
|
|
|||
| from __future__ import print_function | |||
There was a problem hiding this comment.
We shouldn't do this. Just make sure to use parentheses on all print statements.
| Required by signal.signal | ||
|
|
||
| """ | ||
| res = raw_input("Shutdown this visualization server ([y]/n)? ") |
There was a problem hiding this comment.
This will fail in Python 3. Oliver had the fix for this in this file before this PR.
There was a problem hiding this comment.
What are the options then, should I use input()?
There was a problem hiding this comment.
|
Is there any way to have unit tests for this? |
| """ | ||
| res = raw_input("Shutdown this visualization server ([y]/n)? ") | ||
| res = res.lower()[0:1] | ||
| if res == '' or res == 'y': |
There was a problem hiding this comment.
This works even for strings like "Yes"
| def __init__(self, port=8000, scene_file="Null"): | ||
| self.port = port | ||
| self.scene_file = scene_file | ||
| self.directory = "static/" |
There was a problem hiding this comment.
Seems like this should be pass in as an argument.
|
Should I create a custom error for empty scene_file? |
|
i don't think that is necessary. if the user passes in a scene_file that isn't a valid valid file name there will be some other errors that raise, no? |
|
it is for an empty scene file name. |
|
What do you mean by empty? |
|
When user does not pass the scene_file to the server. We should raise an error, no? |
|
If your docstring says: Then you can use the "garbage in, garbage out" rule. |
|
If the user doesn't pass in a required argument they will get an error from Python that says they are missing an argument. |
| def _check_port(self, port): | ||
| soc = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| result = soc.connect_ex(('127.0.0.1', port)) | ||
| if result == 0: |
|
@oliverlee Can you please review this PR. It needs to be merged before my other PR can be reviewed. |
| run_server() | ||
| """ | ||
| res = raw_input("Shutdown this visualization server ([y]/n)? ") | ||
| res = res.lower()[0:1] |
There was a problem hiding this comment.
res.lower()[0] can be used instead.
|
I have addressed all the concerns here. It would be very helpful if someone can please review it. I think it can be merged. Thanks |
|
|
||
| __all__ = ['run_server'] | ||
| if port is None: |
There was a problem hiding this comment.
If you have a default value for port and directory you should just set it in the call signature:
def __init__(self, scene_file, directory='static', port=8000):
Sorry if I've confused you about this.
There was a problem hiding this comment.
But, I don't see how using directory='static' is a good idea. That's why I suggested using directory=None and then if that was true setting directory = os.getcwd().
There was a problem hiding this comment.
Okay , I am updating it.
Static directory gets created in current working directory itself.
|
I'm merging this without tests. I'll raise an issue to fix that. |
Added shutting down calls to the visualization server
Fixes #232
commit message.
in the unit test.)
nosetests) and on Travis CI.pylint, to check your code)
Notes.
follow deprecation cycles.)