observed icon indicating copy to clipboard operation
observed copied to clipboard

Adds type annotations

Open ghost opened this issue 4 years ago • 1 comments

Adds type annotations

Hi! This PR solves the issue #20. It still is a pre PR because I have some things to discuss:

  • I've added typing lib;
  • All tests are passing. Should I add type annotations on tests too ?
  • Whenever the return value is the same as the wrapped function, I added "Any" type annotation (Ex: line 116);
  • I've used "Any" instead of "object" as the type of any object passed as argument. Any enables performing operations and assigning objects to variables, but object doesn't do it. This reference explains it (Ex: line 141);
  • Whenever type checker said "key" could be Tuple or int, I've added Union[int, Tuple]. So, I ask special care when reviewing "key" variable (Ex: line 261);
  • I've added "ObservableBoundMethod" as type on line 570. Since it is being called, should I use "Any" ? The same goes on line 611;
  • I've added "Any" whenever ObservableUnboundMethod could not be accessed

Let's evaluate those points before approving this PR.

ghost avatar May 17 '20 18:05 ghost

I've added typing lib;

Ok!

All tests are passing. Should I add type annotations on tests too ?

Yes, please, but let's do it in a separate pull request to keep reviews easier.

Whenever the return value is the same as the wrapped function, I added "Any" type annotation (Ex: line 116);

Ok, we should look into whether there's some way to do better, i.e. with some form of generic types.

So, I ask special care when reviewing "key" variable (Ex: line 261);

Thanks for calling this out.

I've added "ObservableBoundMethod" as type on line 570. Since it is being called, should I use "Any" ? The same goes on line 611; I've added "Any" whenever ObservableUnboundMethod could not be accessed

I do not quite understand this comment. I'll see if I can figure it out in review.

By the way, you can put comments like this in line by clicking on lines in the "files changed" tab in the pull request.

DanielSank avatar Oct 09 '21 01:10 DanielSank