Added shutting down calls to the visualization server #241

Merged
merged 13 commits into from Jun 18, 2015

Conversation

Projects
None yet
3 participants
@sahilshekhawat
Member

sahilshekhawat commented Jun 17, 2015

Fixes #232

  • There are no merge conflicts.
  • If there is a related issue, a reference to that issue is in the
    commit message.
  • Unit tests have been added for the bug. (Please reference the issue #
    in the unit test.)
  • The tests pass both locally (run nosetests) and on Travis CI.
  • The code follows PEP8 guidelines. (use a linter, e.g.
    pylint, to check your code)
  • The bug fix is documented in the Release
    Notes
    .
  • The code is backwards compatible. (All public methods/classes must
    follow deprecation cycles.)
  • All reviewer comments have been addressed.
@sahilshekhawat

This comment has been minimized.

Show comment
Hide comment
@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

@pydy/pydy-developers This needs a review. Should be a quick one.

Member

sahilshekhawat commented Jun 17, 2015

@pydy/pydy-developers This needs a review. Should be a quick one.

pydy/viz/server.py
@@ -1,37 +1,107 @@
#!/usr/bin/env python
+from __future__ import print_function

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

We shouldn't do this. Just make sure to use parentheses on all print statements.

@moorepants

moorepants Jun 17, 2015

Member

We shouldn't do this. Just make sure to use parentheses on all print statements.

This comment has been minimized.

pydy/viz/server.py
+ """
+ Parameters
+ ----------
+ :param port : integer

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

We try to follow the numpydoc style for docstrings, so this would be:

Parameters
----------
port : integer
    Defines the port on which...
@moorepants

moorepants Jun 17, 2015

Member

We try to follow the numpydoc style for docstrings, so this would be:

Parameters
----------
port : integer
    Defines the port on which...

This comment has been minimized.

pydy/viz/server.py
+
+ Example
+ -------
+ >>> server = Server(scene_file=_scene_json_file)

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

Don't indent here.

@moorepants

moorepants Jun 17, 2015

Member

Don't indent here.

This comment has been minimized.

pydy/viz/server.py
+ Required by signal.signal
+
+ """
+ res = raw_input("Shutdown this visualization server ([y]/n)? ")

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

This will fail in Python 3. Oliver had the fix for this in this file before this PR.

@moorepants

moorepants Jun 17, 2015

Member

This will fail in Python 3. Oliver had the fix for this in this file before this PR.

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

What are the options then, should I use input()?

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

What are the options then, should I use input()?

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

Got it!

pydy/viz/server.py
import webbrowser
-if sys.version_info < (3, 0):

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

This needs to stay in for Python 2/3 compat.

@moorepants

moorepants Jun 17, 2015

Member

This needs to stay in for Python 2/3 compat.

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

I totally missed that. Thanks

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

I totally missed that. Thanks

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

Maybe make a comment in your PR that says why that is there.

@moorepants

moorepants Jun 17, 2015

Member

Maybe make a comment in your PR that says why that is there.

pydy/viz/server.py
+
+ """
+ res = raw_input("Shutdown this visualization server ([y]/n)? ")
+ if res is (None or 'y'):

This comment has been minimized.

@oliverlee

oliverlee Jun 17, 2015

Contributor

Will this eval to True if the user inputs 'Y'?

@oliverlee

oliverlee Jun 17, 2015

Contributor

Will this eval to True if the user inputs 'Y'?

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

Nope, I must add it as well

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

Nope, I must add it as well

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 17, 2015

Member

Is there any way to have unit tests for this?

Member

moorepants commented Jun 17, 2015

Is there any way to have unit tests for this?

@moorepants moorepants added this to the 0.3.0 Release milestone Jun 17, 2015

pydy/viz/server.py
+ """
+ res = raw_input("Shutdown this visualization server ([y]/n)? ")
+ res = res.lower()[0:1]
+ if res == '' or res == 'y':

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

This works even for strings like "Yes"

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

This works even for strings like "Yes"

pydy/viz/server.py
+ def __init__(self, port=8000, scene_file="Null"):
+ self.port = port
+ self.scene_file = scene_file
+ self.directory = "static/"

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

Seems like this should be pass in as an argument.

@moorepants

moorepants Jun 17, 2015

Member

Seems like this should be pass in as an argument.

pydy/viz/server.py
+ """
+ def __init__(self, port=8000, scene_file="Null"):

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

Why isn't "Null" None?

@moorepants

moorepants Jun 17, 2015

Member

Why isn't "Null" None?

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

Because we need to concatenate it with a string and None will raise an error.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

Because we need to concatenate it with a string and None will raise an error.

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

Also, user must know that the there is no file name.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

Also, user must know that the there is no file name.

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

"Null" is just very anit-python-pattern. I don't like it. None should be used and then handled correctly in the code in __init__., but my other suggestion is that this should be a required argument.

@moorepants

moorepants Jun 17, 2015

Member

"Null" is just very anit-python-pattern. I don't like it. None should be used and then handled correctly in the code in __init__., but my other suggestion is that this should be a required argument.

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

And I don't know it yet but the url we get from "Null" can be used to give user an option to choose the json files like in ipython notebook.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

And I don't know it yet but the url we get from "Null" can be used to give user an option to choose the json files like in ipython notebook.

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

We can worry about that when the feature is implemented. I think scene_file should be a required arg.

@moorepants

moorepants Jun 17, 2015

Member

We can worry about that when the feature is implemented. I think scene_file should be a required arg.

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

Should I change it to None?

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

Should I change it to None?

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

or make it a required argument?

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

or make it a required argument?

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

Seems to me like it is a requirement. What use is the server if I don't have a scene file? I guess you could load it from a url ?scene_file=... after running the server. If you want to support that use case, then you can make it a kwarg and =None or =""but not="Null". I'd preferNone` as it follows common python patterns.

@moorepants

moorepants Jun 17, 2015

Member

Seems to me like it is a requirement. What use is the server if I don't have a scene file? I guess you could load it from a url ?scene_file=... after running the server. If you want to support that use case, then you can make it a kwarg and =None or =""but not="Null". I'd preferNone` as it follows common python patterns.

pydy/viz/server.py
+ """
+ def __init__(self, port=8000, scene_file="Null"):
+ self.port = port
+ self.scene_file = scene_file

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

My suggestion in the previous commit was to have this call sig:

def __init__(scene_file, directory=None, port=8000)

The scene file should be required and the directory can default to the cwd if None is passed in.

@moorepants

moorepants Jun 17, 2015

Member

My suggestion in the previous commit was to have this call sig:

def __init__(scene_file, directory=None, port=8000)

The scene file should be required and the directory can default to the cwd if None is passed in.

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

I will add that in my new pr on static directory creation. Trying to make it atomic

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

I will add that in my new pr on static directory creation. Trying to make it atomic

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

But this is a whole new server code. It seems atomic to add this.

@moorepants

moorepants Jun 17, 2015

Member

But this is a whole new server code. It seems atomic to add this.

pydy/viz/server.py
- print(url)
- webbrowser.open(url)
- httpd.serve_forever()
+ def run_server(self):

This comment has been minimized.

@moorepants

moorepants Jun 17, 2015

Member

What happens if port 8000 is taken?

@moorepants

moorepants Jun 17, 2015

Member

What happens if port 8000 is taken?

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

It throws an error. This needs to be fixed.

@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

It throws an error. This needs to be fixed.

@sahilshekhawat

This comment has been minimized.

Show comment
Hide comment
@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

Should I create a custom error for empty scene_file?

Member

sahilshekhawat commented Jun 17, 2015

Should I create a custom error for empty scene_file?

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 17, 2015

Member

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?

Member

moorepants commented Jun 17, 2015

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?

@sahilshekhawat

This comment has been minimized.

Show comment
Hide comment
@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

it is for an empty scene file name.

Member

sahilshekhawat commented Jun 17, 2015

it is for an empty scene file name.

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 17, 2015

Member

What do you mean by empty?

Member

moorepants commented Jun 17, 2015

What do you mean by empty?

@sahilshekhawat

This comment has been minimized.

Show comment
Hide comment
@sahilshekhawat

sahilshekhawat Jun 17, 2015

Member

When user does not pass the scene_file to the server. We should raise an error, no?

Member

sahilshekhawat commented Jun 17, 2015

When user does not pass the scene_file to the server. We should raise an error, no?

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 17, 2015

Member

If your docstring says:

scene_file : string
    A valid path to a file in the ``directory``.

Then you can use the "garbage in, garbage out" rule.

Member

moorepants commented Jun 17, 2015

If your docstring says:

scene_file : string
    A valid path to a file in the ``directory``.

Then you can use the "garbage in, garbage out" rule.

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 17, 2015

Member

If the user doesn't pass in a required argument they will get an error from Python that says they are missing an argument.

Member

moorepants commented Jun 17, 2015

If the user doesn't pass in a required argument they will get an error from Python that says they are missing an argument.

@sahilshekhawat sahilshekhawat referenced this pull request Jun 18, 2015

Closed

Pr240 fixing static directory creation issue #242

0 of 8 tasks complete
pydy/viz/server.py
+ 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:

This comment has been minimized.

@oliverlee

oliverlee Jun 18, 2015

Contributor

return result == 0

@oliverlee

oliverlee Jun 18, 2015

Contributor

return result == 0

This comment has been minimized.

@sahilshekhawat

This comment has been minimized.

Show comment
Hide comment
@sahilshekhawat

sahilshekhawat Jun 18, 2015

Member

@oliverlee Can you please review this PR. It needs to be merged before my other PR can be reviewed.

Member

sahilshekhawat commented Jun 18, 2015

@oliverlee Can you please review this PR. It needs to be merged before my other PR can be reviewed.

pydy/viz/server.py
- run_server()
+ """
+ res = raw_input("Shutdown this visualization server ([y]/n)? ")
+ res = res.lower()[0:1]

This comment has been minimized.

@oliverlee

oliverlee Jun 18, 2015

Contributor

res.lower()[0] can be used instead.

@oliverlee

oliverlee Jun 18, 2015

Contributor

res.lower()[0] can be used instead.

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 18, 2015

Member

Yup! changing.

@sahilshekhawat

sahilshekhawat Jun 18, 2015

Member

Yup! changing.

pydy/viz/server.py
+ Parameters
+ ----------
+ port : integer
+ Defines the port on which the server will run.

This comment has been minimized.

@oliverlee

oliverlee Jun 18, 2015

Contributor

This isn't the case if the port is taken.

@oliverlee

oliverlee Jun 18, 2015

Contributor

This isn't the case if the port is taken.

pydy/viz/server.py
+ if directory:

This comment has been minimized.

@oliverlee

oliverlee Jun 18, 2015

Contributor

Comparisons to singletons like None should always be done with is or is not:
if directory is not None

@oliverlee

oliverlee Jun 18, 2015

Contributor

Comparisons to singletons like None should always be done with is or is not:
if directory is not None

@sahilshekhawat

This comment has been minimized.

Show comment
Hide comment
@sahilshekhawat

sahilshekhawat Jun 18, 2015

Member

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

Member

sahilshekhawat commented Jun 18, 2015

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

pydy/viz/server.py
-__all__ = ['run_server']
+ if port is None:

This comment has been minimized.

@moorepants

moorepants Jun 18, 2015

Member

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.

@moorepants

moorepants Jun 18, 2015

Member

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.

This comment has been minimized.

@moorepants

moorepants Jun 18, 2015

Member

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().

@moorepants

moorepants Jun 18, 2015

Member

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().

This comment has been minimized.

@sahilshekhawat

sahilshekhawat Jun 18, 2015

Member

Okay , I am updating it.
Static directory gets created in current working directory itself.

@sahilshekhawat

sahilshekhawat Jun 18, 2015

Member

Okay , I am updating it.
Static directory gets created in current working directory itself.

@moorepants

This comment has been minimized.

Show comment
Hide comment
@moorepants

moorepants Jun 18, 2015

Member

I'm merging this without tests. I'll raise an issue to fix that.

Member

moorepants commented Jun 18, 2015

I'm merging this without tests. I'll raise an issue to fix that.

moorepants added a commit that referenced this pull request Jun 18, 2015

Merge pull request #241 from sahilshekhawat/clean_exit_of_server
Added shutting down calls to the visualization server

@moorepants moorepants merged commit 756f80a into pydy:master Jun 18, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment