Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type hints #966

Closed
lavigne958 opened this issue Dec 8, 2021 · 26 comments
Closed

Add type hints #966

lavigne958 opened this issue Dec 8, 2021 · 26 comments
Assignees
Milestone

Comments

@lavigne958
Copy link
Collaborator

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 lavigne958 self-assigned this Dec 8, 2021
@lavigne958 lavigne958 added this to the 5.1.0 milestone Dec 12, 2021
@lavigne958 lavigne958 removed this from the 5.1.0 milestone Dec 22, 2021
@lavigne958 lavigne958 added this to the 5.2.0 milestone Jan 6, 2022
@lavigne958
Copy link
Collaborator Author

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 🙂

@lavigne958 lavigne958 removed this from the 5.2.0 milestone Feb 9, 2022
@Congee
Copy link

Congee commented Mar 1, 2022

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.

@lavigne958
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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 lavigne958 added this to the 6.0.0 milestone Mar 6, 2022
@lavigne958 lavigne958 mentioned this issue Apr 25, 2022
@OskarBrzeski
Copy link
Contributor

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

@lavigne958
Copy link
Collaborator Author

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

@OskarBrzeski
Copy link
Contributor

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.

@lavigne958
Copy link
Collaborator Author

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.

@OskarBrzeski
Copy link
Contributor

I'll try client.py

@shahriyardx
Copy link

Any progress on this?

@lavigne958
Copy link
Collaborator Author

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
Copy link
Collaborator Author

lavigne958 commented May 16, 2023

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

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

@butvinm
Copy link
Contributor

butvinm commented May 17, 2023

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?

@lavigne958
Copy link
Collaborator Author

lavigne958 commented May 17, 2023

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

@butvinm
Copy link
Contributor

butvinm commented May 18, 2023

Ok, I can try adding types for utils

@lavigne958
Copy link
Collaborator Author

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
Copy link
Collaborator Author

I'll take the worksheet.py

@lavigne958
Copy link
Collaborator Author

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.

@butvinm
Copy link
Contributor

butvinm commented May 28, 2023

@lavigne958 I made draft PR but need discuss some issues.
#1196 (comment)

@lavigne958
Copy link
Collaborator Author

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

lavigne958 added a commit that referenced this issue Jun 8, 2023
@lavigne958
Copy link
Collaborator Author

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.

@alifeee
Copy link
Collaborator

alifeee commented Aug 13, 2023

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

@lavigne958
Copy link
Collaborator Author

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

I just updated the command to have better output.

@alifeee
Copy link
Collaborator

alifeee commented Aug 13, 2023

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

def convert_colors_to_hex_value(red=0, green=0, blue=0):

@lavigne958
Copy link
Collaborator Author

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

def convert_colors_to_hex_value(red=0, green=0, blue=0):

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 😆 ) 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.

@alifeee
Copy link
Collaborator

alifeee commented Oct 19, 2023

replaced by #1321

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants