Skip to content

Client.py and error no 35 fix#2

Merged
mwalsh161 merged 4 commits intomwalsh161:masterfrom
cpeng365:master
Jan 14, 2020
Merged

Client.py and error no 35 fix#2
mwalsh161 merged 4 commits intomwalsh161:masterfrom
cpeng365:master

Conversation

@cpeng365
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Owner

@mwalsh161 mwalsh161 left a comment

Choose a reason for hiding this comment

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

A few pretty minor changes. All print statements should be logger statements and all return values should be the actual values, not formatted strings.

Comment thread client.py
sock.sendall((urllib.parse.quote_plus(handshake)+'\n').encode())

# Look for the response
resp = self.__recv(sock)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Might be worth checking that the response is the expected response.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You mean checking if the response is "ack"?
I added a line "assert resp == 'ack', '%s does not exist' % module". Let me know if that works.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yep - except I would say a more general error; "Wan't able to get an acknowledgement from the server" (your error message makes an assumption that we don't need to make)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it! Just changed it.

Comment thread client.py Outdated
Comment thread client.py Outdated
Copy link
Copy Markdown
Owner

@mwalsh161 mwalsh161 left a comment

Choose a reason for hiding this comment

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

Looks good!

@mwalsh161 mwalsh161 merged commit 64bd4b6 into mwalsh161:master Jan 14, 2020
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.

2 participants