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

Feature/add type hints worksheets #1254

Merged
merged 6 commits into from
Jul 31, 2023

Conversation

lavigne958
Copy link
Collaborator

Add type hints for Worksheet object.

It's complete, but the majority's of it is done. It could be improved with time and when all files have been typed.

It passez the test suite, it passes mypy (some errors are still showing due to missing type annotations on some files).

Later on some of the types here will be improved when I type Spreadsheet object and once merged I will be able to better type the functions in utils.py etc.

@lavigne958 lavigne958 requested a review from alifeee July 15, 2023 09:54
@lavigne958 lavigne958 self-assigned this Jul 15, 2023
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.

This is a big change! You've done great work with it ❤

I have added a few comments.

I also wonder: JSONAnyType is returned by a lot of the functions like update_title. This seems a bit vague. I am not sure where else it is used, but when we return response (which we do a lot), perhaps we could change the name to HTTPResponseType, or the default import requests response type? In my opinion this would make it more obvious when a method is returning the (raw) HTTP response, rather than a "useful" JSON object.

tests/cell_test.py Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
@alifeee alifeee added this to the 6.0.0 milestone Jul 16, 2023
Copy link
Collaborator Author

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

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

thanks for the review that's a good catch. I added the missing types and updated the wrong types.

gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
tests/cell_test.py Show resolved Hide resolved
@lavigne958
Copy link
Collaborator Author

lavigne958 commented Jul 18, 2023

lso wonder: JSONAnyType is returned by a lot of the functions like update_title. This seems a bit vague. I am not sure where else it is used, but when we return response (which we do a lot), perhaps we could change the name to HTTPResponseType, or the default import requests response type? In my opinion this would make it more obvious when a method is returning the (raw) HTTP response, rather than a "useful" JSON object.

We could but when you look at the file http_client.py you'll see that the response is the actual JSON response we do:

res = self.request.post(....)
return res.json()

so that's the JSON reponse.

We can rename it 'JSONResponse', that is more helpful and type it: MuttableMapping[str. Any]. I use Any for the value because it can be anything, int, str, list of str or int mixed, dict etc.

The responses can vary, when it's a value from a get it should be this: https://developers.google.com/sheets/api/reference/rest/v4/spreadsheets.values#ValueRange
when it's an update it could be something like: https://developers.google.com/sheets/api/reference/rest/v4/spreadsheets/response

I thought may be we want to be exaustif and create a new type with all possible values inside from the API but that means we need to maintain it ! and we don't look at the response very often we simply return it to the user so no point to me but it's open for discussion 😆

@alifeee
Copy link
Collaborator

alifeee commented Jul 20, 2023

I thought may be we want to be exaustif and create a new type with all possible values inside from the API but that means we need to maintain it ! and we don't look at the response very often we simply return it to the user so no point to me but it's open for discussion 😆

This sounds like a lot to me, and probably not worth it...! Just having the JSON Any type makes sense, but perhaps with a different name like JSONResponse :)

@lavigne958
Copy link
Collaborator Author

I thought may be we want to be exaustif and create a new type with all possible values inside from the API but that means we need to maintain it ! and we don't look at the response very often we simply return it to the user so no point to me but it's open for discussion laughing

This sounds like a lot to me, and probably not worth it...! Just having the JSON Any type makes sense, but perhaps with a different name like JSONResponse :)

agreed we can rename it JSONResponse.
We can merge this PR, then after it I can start working on better typing between files now that they will be almost all typed.
I'll may be able to add some typed-dict etc too. We'll see 🙃

@lavigne958
Copy link
Collaborator Author

Alright this is a go for merge ! 🥳

@lavigne958 lavigne958 merged commit ac6b678 into feature/release_6_0_0 Jul 31, 2023
@lavigne958 lavigne958 deleted the feature/add_type_hints_worksheets branch July 31, 2023 14:40
@alifeee alifeee mentioned this pull request Jan 26, 2024
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