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

Initial typing in client.py #1159

Merged
merged 6 commits into from
Jan 10, 2023
Merged

Initial typing in client.py #1159

merged 6 commits into from
Jan 10, 2023

Conversation

OskarBrzeski
Copy link
Contributor

Not fully certain about some of the type hints, especially regarding Client.request().

@lavigne958
Copy link
Collaborator

Hi you can ise the following command to format the entire code and pass the linter: tox -e format

@OskarBrzeski
Copy link
Contributor Author

The issue seems to be with the imports in exceptions.py and cell.py. Should I commit those changes to this PR?

Also, tox sees this line passenv = GS_RECORD_MODE GS_CREDS_FILENAME APPDATA in tox.ini as an error because it wants commas instead of spaces. Should I change this too?

@lavigne958
Copy link
Collaborator

I see, I did not finish my fishes on gspread/utils.py but I have the fix for those errors on the files you did not change. I will push the fixes on those files so you can rebase your pr (or merge the branch feature/release_6_0_0 in your branch or cherry-pick the commits) then it should pass the CI test.

I'll let you know when you can rebase. sorry for the inconvenience

@lavigne958
Copy link
Collaborator

✔️ I pushed the necessary changes to make the CI green again on the branch feature/release_6_0_0. you can rebase your branch onto it.

Copy link
Collaborator

@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.

thank you for this contribution, here are a few comments, mostly about JSON to be typed as Any because it's a nightmare to maintain.

gspread/client.py Outdated Show resolved Hide resolved
gspread/client.py Outdated Show resolved Hide resolved
gspread/client.py Outdated Show resolved Hide resolved
gspread/client.py Outdated Show resolved Hide resolved
gspread/client.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

last changes

gspread/client.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Awesome. Looks good to me. I'll run mypy on it when I get z Chance

@lavigne958 lavigne958 merged commit e23d17e into burnash:feature/release_6_0_0 Jan 10, 2023
@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