gspread icon indicating copy to clipboard operation
gspread copied to clipboard

Add type hints

Open lavigne958 opened this issue 3 years ago • 4 comments

Add type hints to gspread. Add mypy to check types in the CI.

This allows developer to get type hints from their editors when developing. This allows users to get the type hints from their editors when using gspread and have a clear hint on the return value of each function, the type of each expected argument. It should make the use of gspread much simpler and the development of gspread easier.

lavigne958 avatar Dec 08 '21 22:12 lavigne958

remove that issue from milestone 5.2.0.

It is much longer than expected. Tried the bottom up approach and it's not right, tried having a strict typing and it does not work well with JSON (see issue python/typing#182)

Will do it again but using a top down approach by typing

  1. every response from the library as a typedDict see PEP 589
  2. from there type every method/function manipulating the response from Spreadsheet API
  3. Will not use strict typing and let mypy bring up only what needs to be typed.

Postone that issue for later, will work on it a background task.

Any help is welcome :slightly_smiling_face:

lavigne958 avatar Feb 09 '22 18:02 lavigne958

What about gradually typing this library? We can write Json = typing.Any to skip typing json for now and type other stuff as much as possible. It is better than nothing for users.

Congee avatar Mar 01 '22 21:03 Congee

I tried that in my previous attempt and it does not help. We access keys of z dict and the values may be an int or a str and it does not help us identifying the exact type for the next functions.

I will see may be i can simply add types to some user facing functions and leave the rest for later, that is an other option too 🤔

lavigne958 avatar Mar 02 '22 07:03 lavigne958

I found the bottleneck of this feature:

the spreadsheet class can return instance of the worksheet class. The worksheet class has a link to the spreadsheet parent object to call every low level methods (batch_get, batch_update, ...)

This brings circular dependency.

In order to simply type the gspread library we'll need to:

  • move any low level method to the worksheet class
  • break the circular dependency between spreadsheet and worksheet.

From there we should be able to add some basic typing.

Will plan this for release 6.0.0

lavigne958 avatar Mar 06 '22 20:03 lavigne958

How much progress has been made for this issue? Is it possible to see this progess at the current moment?

OskarBrzeski avatar Dec 26 '22 22:12 OskarBrzeski

Hi it's starting, I have a branch where I add commits for the 6.0.0 release.

You can follow it here: https://github.com/burnash/gspread/commits/feature/release_6_0_0

lavigne958 avatar Dec 27 '22 09:12 lavigne958

I could try helping towards this. If there are any particular files you'd like me to type hint, I'll gladly give it a go.

OskarBrzeski avatar Dec 27 '22 17:12 OskarBrzeski

That would be helpful thanks, I am currently typing the file https://github.com/burnash/gspread/blob/feature/release_6_0_0/gspread/utils.py

which is very heavy to do (on my spare time), but over half of it is done.

you can try adding typing to https://github.com/burnash/gspread/blob/feature/release_6_0_0/gspread/auth.py , there aren't many functions but they rely on untyped libraries :-(

or you can try: https://github.com/burnash/gspread/blob/feature/release_6_0_0/gspread/client.py

which is heavier but we still need to do it.

Let me know which one you do, I'll do the other one.

then open your PR and target the branch https://github.com/burnash/gspread/blob/feature/release_6_0_0

I run mypy as follow mypy --strict ./gspread/utils.py then it must pass all checks, unless we can't do anything about (untyped underlying libraries etc)

Thank you for helping, looking forward to review your PR.

lavigne958 avatar Dec 27 '22 23:12 lavigne958

I'll try client.py

OskarBrzeski avatar Dec 28 '22 12:12 OskarBrzeski

Any progress on this?

shahriyardx avatar May 08 '23 21:05 shahriyardx

Hi yes, you can follow the branch https://github.com/burnash/gspread/tree/feature/release_6_0_0

I push on it every changes that impacts the typing.

i am still facing issues with refactoring for the new clients and HTTP clients in order to prevent cyclic imports.

lavigne958 avatar May 09 '23 08:05 lavigne958

Basic typing progress for each file (basic == no use of --strict):

  • [x] __init__.py (no typing to do)
  • [x] auth.py -> passes with --strict
  • [x] cell.py -> passes with --strict
  • [x] client.py -> does not pass --strict but it will after utils.py is done
  • [x] exceptions.py passes with --strict
  • [x] http_client.py does not pass --strict but it will after utils.py is done
  • [ ] spreadsheet.py
  • [x] url.py passes with --strict
  • [x] utils.py
  • [x] worksheet.py

lavigne958 avatar May 16 '23 23:05 lavigne958

Went here to add types for this library and found that someone has finally done it. Thank you for your effort. Have you also tried using MonkeyType?

butvinm avatar May 17 '23 15:05 butvinm

Hi yes I'm working on it, on the branch feature/release_6_0_0

You're welcome to participate, i tried to take it step by step.

Above is a lost of what is left to do. Just let le know which one you wish to tackle.

Interestingly the longest/hardest was the utils.py 😯

I have not tried this tool, but it will take a look

lavigne958 avatar May 17 '23 22:05 lavigne958

Ok, I can try adding types for utils

butvinm avatar May 18 '23 12:05 butvinm

Ok, I can try adding types for utils

Great thank you 😃 open a PR when ready of if you're stuck we'll work from here.

lavigne958 avatar May 19 '23 10:05 lavigne958

I'll take the worksheet.py

lavigne958 avatar May 21 '23 11:05 lavigne958

Btw @butvinm please start your branch from feature/release_6_0_0 as this is where we're going to merge your changes, thank you.

lavigne958 avatar May 27 '23 11:05 lavigne958

@lavigne958 I made draft PR but need discuss some issues. https://github.com/burnash/gspread/pull/1196#issue-1729122233

butvinm avatar May 28 '23 03:05 butvinm

I see, I'll have a look, it will take some time.

lavigne958 avatar May 28 '23 09:05 lavigne958

Current status:

  • Need to change all the named tuples for Enums
  • finish what can be done in gspread/worksheet.py
  • work on cross file references (typing that conflict from cross files, find the right typing for them and apply fix on both files) like for the typing of some function in gspread/utils.py that are used in gspread/worksheet.py.

lavigne958 avatar Jun 16 '23 15:06 lavigne958

@lavigne958 will you add a type-checking command to the CI?

alifeee avatar Aug 13 '23 09:08 alifeee

Yes it's already the case ok branch release_6.0

I just updated the command to have better output.

lavigne958 avatar Aug 13 '23 09:08 lavigne958

what does it check?

For example, this commit passed the CI, but the functions in the commit do not have typing, so I would expect it to fail

https://github.com/burnash/gspread/blob/30ccb5a7fb1c3f21bf556de725ef9c3f0bb9ed01/gspread/utils.py#L737

alifeee avatar Aug 13 '23 10:08 alifeee

what does it check? It checks that the given annotations are correct and match from function return to function argument ets.

For example, this commit passed the CI, but the functions in the commit do not have typing, so I would expect it to fail

https://github.com/burnash/gspread/blob/30ccb5a7fb1c3f21bf556de725ef9c3f0bb9ed01/gspread/utils.py#L737

Currently I wish the branch feature/release_6.0.0 to pass the CI in order to merge. As the typing is not complete, I set it up so it runs but it does not prevent the CI from a succeeding. When we are completely typed we can enforce it as a proper hard stop.

There are 2 types of run in mypy the regular run mode and the --strict mode. the later one is very strict (hence the name :laughing: ) and can be too strict. so in regular mode if a function is not typed and is not used in a typed context it's ok.

lavigne958 avatar Aug 15 '23 22:08 lavigne958

replaced by #1321

alifeee avatar Oct 19 '23 11:10 alifeee