Add typing to worker module#38
Conversation
| name: str | list[str], | ||
| default: Any = None, | ||
| message: str | None = None, | ||
| ) -> Any: |
There was a problem hiding this comment.
Without validation I'm not convinced it is worth it to narrow the return type beyond Any, but we could narrow it down to a JSON subset dict | list | str | int | float | bool | None (https://docs.python.org/3/library/json.html#json.JSONEncoder).
Other option is to break compatibility and add a function with a expected_type param that errors if an isinstance check fails, for example.
| else: | ||
| job_directory = "/job" | ||
| self.job_directory = job_directory | ||
| self.job_directory: str | None = job_directory |
There was a problem hiding this comment.
May look unnecessary but without this explicit annotation it clashes with L40 self.job_directory = None
|
|
||
|
|
||
| class Worker: | ||
| READ_TIMEOUT = 3 # seconds |
There was a problem hiding this comment.
This looks unused. Again, chance to break compatibility
| return default | ||
|
|
||
| def error(self, message, ensure_ascii=False): | ||
| def error(self, message: str, ensure_ascii: bool = False) -> NoReturn: |
There was a problem hiding this comment.
As the issue states, I think here is 80% of the benefit of typing this class
| self.__write_output(output, ensure_ascii=ensure_ascii) | ||
|
|
||
| def run(self): | ||
| def run(self) -> None: |
There was a problem hiding this comment.
Return -> None might be opinionated here. We may want users to have more freedom if run should ever return something
1 participant
This PR addresses #37.
I've tried to keep types wide for now.
TODO: