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

Call stop before raising SocketError in _read_loop #52

Merged
merged 5 commits into from
Apr 8, 2017
Merged

Call stop before raising SocketError in _read_loop #52

merged 5 commits into from
Apr 8, 2017

Conversation

agberk
Copy link
Contributor

@agberk agberk commented Mar 12, 2017

I've moved the socket cleanup code into stop and called it before raising SocketError.

I had to modify one of the tests since it was expecting a SocketError but the stop code was causing a different exception to be thrown as the socket was mocked and it was trying to call a non-existent method.

Maybe it would have been better to mock the shutdown and close methods on the socket instead?

@coveralls
Copy link

coveralls commented Mar 12, 2017

Coverage Status

Coverage increased (+0.005%) to 95.149% when pulling 543befa on agberk:issue-45 into 0563477 on liampauling:master.

@liampauling
Copy link
Member

liampauling commented Mar 29, 2017

Sorry for the delay but how about the following to keep things DRY

except (socket.timeout, socket.error) as e:
    self.stop()
    raise SocketError('[Connect: %s]: Socket %s' % (self.unique_id, e))

@agberk
Copy link
Contributor Author

agberk commented Mar 31, 2017

Yup sounds sensible - I'll add another commit.

@coveralls
Copy link

coveralls commented Mar 31, 2017

Coverage Status

Coverage decreased (-0.002%) to 95.142% when pulling b82d36b on agberk:issue-45 into 0563477 on liampauling:master.

@liampauling liampauling merged commit b82d36b into betcode-org:master Apr 8, 2017
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

3 participants