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 some missing typing in code #1448

Merged
merged 2 commits into from
Apr 2, 2024
Merged

Add some missing typing in code #1448

merged 2 commits into from
Apr 2, 2024

Conversation

lavigne958
Copy link
Collaborator

Introduce a lot of Any Typing is most method in the Spreadsheet object return some JSON object and we can't type it.

ref: #1430

@lavigne958 lavigne958 requested a review from alifeee April 1, 2024 21:49
@lavigne958 lavigne958 self-assigned this Apr 1, 2024
@lavigne958 lavigne958 force-pushed the feature/add_more_typing branch from fac5e85 to 6b15a66 Compare April 1, 2024 21:51
ref: #1430

Signed-off-by: Alexandre Lavigne <[email protected]>
@lavigne958 lavigne958 force-pushed the feature/add_more_typing branch from 6b15a66 to f591810 Compare April 1, 2024 21:57
Copy link
Collaborator

@alifeee alifeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a lot to this ! you found some inspiration and a love of types. it is good effort :)

I have left a few comments

Some questions:

why was so much left untyped?

is there a json type? Is it Response? Why do many of the return types use Any instead of Response?

gspread/client.py Show resolved Hide resolved
gspread/spreadsheet.py Show resolved Hide resolved
gspread/spreadsheet.py Outdated Show resolved Hide resolved
gspread/spreadsheet.py Show resolved Hide resolved
@lavigne958
Copy link
Collaborator Author

there is a lot to this ! you found some inspiration and a love of types. it is good effort :)

I have left a few comments

Some questions:

why was so much left untyped?

I believe when we merge different branches we picked the code from the untyped branch and left it as-is.

is there a json type? Is it Response? Why do many of the return types use Any instead of Response?

No there is not 😢
No Response is a a type from the requests library, it represents the Response class that is returned from a response from an HTTP request.

We return Any because returning a JSON type is too complex.

Mypy works with a static type, and no type heritage. Meaning: when you define a type like Mapping[str, Any] you can't tell mypy that the Any for the possible values could be the exact same type as itself.

When you want to type some JSON and this JSON is a Dict with any kind of value, you must define all possible type you could expect: List[...], Mapping[..., ...], str, int, .... and for each List[X] you must define what could be inside, so again you must provide all possible types.... In the end it's exactly the same as Any which in short means -> it could be any type.

there is a huge github issue on the official python/typing repo about JSON, the very first comment shows a very clear example of what we need (but don't have): python/typing#182

In the mean time, we are left with Any

@lavigne958
Copy link
Collaborator Author

Now that I checked the issue again, I found this ! https://github.com/kevinheavey/jsonalias

which could do the trick.

I would recommend we first merge this like this, then later in a second PR we change all Any to some JSONType

@alifeee
Copy link
Collaborator

alifeee commented Apr 2, 2024

all great reasoning. mypy has whims and we must conform to them

lgtm :)

@lavigne958 lavigne958 merged commit 4ef1fb8 into master Apr 2, 2024
10 checks passed
@lavigne958 lavigne958 deleted the feature/add_more_typing branch April 2, 2024 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants