On 03/13/2014 10:34 PM, Sheldon wrote:
On 03/13/2014 09:15 PM, CrÃstian Viana wrote:
This seems to be a nice tool to improve code quality! But we need to check its changes before applying them. Some of them do not make sense. Take a look at two examples below:
Am 13-03-2014 00:13, schrieb shaohef@linux.vnet.ibm.com:
def vm_start(self, name): self._get_vm(name).info['state'] = 'running' - info = self._get_vm(name).info + self._get_vm(name).info The variable "info" was actually unused, but removing the assignment is not enough. Now we have a useless call to _get_vm. The solution here is to remove that line completely. good point. we can remove this whole line. will fix in the next version.
def vm_stop(self, name): self._get_vm(name).info['state'] = 'shutoff' @@ -703,7 +703,7 @@ class MockModel(object): def add_task(self, target_uri, fn, opaque=None): id = self.next_taskid self.next_taskid = self.next_taskid + 1 - task = AsyncTask(id, target_uri, fn, self.objstore, opaque) + AsyncTask(id, target_uri, fn, self.objstore, opaque) This situation is similar to the above one, but looking at the Async class code, it seems its constructor actually does something important other than creating the object - it starts a thread. So in this case, I believe this solution is OK. But I never used the Async class, so I may be missing something.
yes AsyncTask starts a thread. Ming can you help to check it?
Should AsyncTask has an explicit start method on it, separating construction from execution? It would make the behavior explicit. -- Adam King <rak@linux.vnet.ibm.com> IBM CSI