Type annotations fixed #200
Conversation
|
Hi @CCCodes, thank you for updating the PR based on my previous comments! Apologies for the delay in response, I'll be reviewing it now |
tango.py
Outdated
| @@ -209,7 +209,7 @@ def getInfo(self): | |||
| # | |||
| # Helper functions | |||
| # | |||
| def resetTango(self, vmms): | |||
| def resetTango(self, vmms: list) -> None: | |||
There was a problem hiding this comment.
list should be List[<some type>] instead, and can be imported with from typing import List
There was a problem hiding this comment.
Great job overall and thanks so much for your contributions! There are some compound types and custom datatypes whose annotations are wrong but it's also partially the fault of the lack of a CI pipeline in Tango which means that you won't really be able to check if they are actually working. Could you address the comments, and once those are done it should be good to go!
| @@ -313,7 +315,7 @@ def makeDead(self, id, reason): | |||
| self.log.debug("makeDead| Released lock to job queue.") | |||
| return status | |||
|
|
|||
| def getInfo(self): | |||
| def getInfo(self) -> dict: | |||
There was a problem hiding this comment.
Dict should be capitalized and contain the type mapping, see https://docs.python.org/3/library/typing.html#typing.Dict
| @@ -353,7 +355,7 @@ def getNextPendingJob(self): | |||
| self.log.debug("getNextPendingJob| Released lock to job queue.") | |||
| return job | |||
|
|
|||
| def reuseVM(self, job): | |||
| def reuseVM(self, job: TangoJob) -> Optional['vm']: | |||
There was a problem hiding this comment.
'vm' is probably not a valid type
tangoObjects.py
Outdated
| from queue import Queue | ||
| from config import Config | ||
|
|
||
| redisConnection = None | ||
|
|
||
|
|
||
| def getRedisConnection(): | ||
| def getRedisConnection() -> redis.StrictRedis: |
There was a problem hiding this comment.
Figuring out the type of this is tricky, it's actually redis.client.Redis. You can do it by installing mypy, creating a test python file (say test.py), adding the following stub:
import redis
reveal_type(redis.StrictRedis(host='localhost', port=6379, db=0))
reveal_type is a feature from mypy that has an example here: https://mypy.readthedocs.io/en/stable/cheat_sheet_py3.html#when-you-re-puzzled-or-when-things-are-complicated
tangoObjects.py
Outdated
| self.__db.delete(self.key) | ||
|
|
||
| # This is an abstract class that decides on | ||
| # if we should initiate a TangoRemoteDictionary or TangoNativeDictionary | ||
| # Since there are no abstract classes in Python, we use a simple method | ||
| def TangoDictionary(object_name): | ||
| def TangoDictionary(object_name) -> 'TangoDictionary': |
There was a problem hiding this comment.
This should be tangoObjects.TangoRemoteDictionary
tangoObjects.py
Outdated
| keys = map(lambda key : key.decode(), self.r.hkeys(self.hash_name)) | ||
| return list(keys) | ||
|
|
||
| def values(self): | ||
| def values(self) -> list: |
There was a problem hiding this comment.
Same comment on list as before
|
Thanks for the feedback, I will work on the changes now! |
There was a problem hiding this comment.
Thanks for making the changes @CCCodes! I looked through the changes again and found a few more issues but I'll be able to take care of them. Merging changes into a temporary branch first in order to fix these issues. Once again thanks for your contributions!
Fixes #185.
Changes proposed in this PR:
Add type annotations for
Some type annotations are best guesses because it was hard to determine exactly what types were acceptable. I ran all unittests on it and got the same results with / and without the annotations, so there were no problems in those cases.