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

Fix asynchrony bugs in dc_rack_server.py #293

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix asynchrony bugs in dc_rack_server.py #293

wants to merge 2 commits into from

Conversation

amopremcak
Copy link

@DanielSank Thanks for taking the time to write up these notes. I followed your advice and separated the bug fixes from the new features. I must have added and subtracted a bunch of blank lines from this file as I modified it to suit our needs. In any case, I should have payed more attention to these changes when I submitted the initial pull request. I have a decent amount of experience with git, but I am fairly new to pull requests. Seems like a nice way of controlling what changes go into your master branch.

@@ -607,7 +605,7 @@ def state(self):

def strState(self):
"""Returns the channel state as a list of strings"""
s = list(self.state)
s = list(self.state()) # self.state is a method, not an attribute...
Copy link
Member

Choose a reason for hiding this comment

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

Remove the comment.

@DanielSank
Copy link
Member

The changes all look good except for that write([1L]) which has been removed. It looks quite deliberate so I want to know why it was there before we get rid of it.

@amopremcak
Copy link
Author

I added the changes to the fix_dc_rack_server branch. Sorry for the delay, I've been caught up with finals.

@DanielSank
Copy link
Member

I say LGTM. However, I think we should get one from @zchen088 too since this is important and I think he's the real expert with this part of the system.

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