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

Update camera poller to close the camera when polling is complete. #127

Merged
merged 2 commits into from
Apr 8, 2017

Conversation

JeetShetty
Copy link
Owner

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 80.662% when pulling a5f5608 on close_camera into 1d3d410 on master.

Copy link
Collaborator

@mtlynch mtlynch left a comment

Choose a reason for hiding this comment

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

LGTM just one note

def stop(self):
"""End worker polling and close camera."""
self._stopped.set()
self._sensor.close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest:

        self._sensor.close()
        super(_CameraPollWorker, self).stop()

Because then if we ever add anything to the base class' close, we don't have to remember to add it here independently. Also I'm suggesting calling the parent's close second because it's the opposite of a constructor. The constructor we want to call the parent first because the child might have dependencies on the parent's object initialization. So when we close, we want to close the child's things first, then close the parent's in case the child has any dependencies on the parent's objects staying alive.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh yeah! Good points.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.005%) to 80.556% when pulling cbff78a on close_camera into 1d3d410 on master.

@JeetShetty JeetShetty merged commit 89f729f into master Apr 8, 2017
@JeetShetty JeetShetty deleted the close_camera branch April 8, 2017 00:28
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.

3 participants