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.

Sign up for GitHub

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

Jump to bottom

bugfix/client export #1392

Merged
merged 4 commits into from
Jan 31, 2024
Merged

bugfix/client export #1392

merged 4 commits into from
Jan 31, 2024

Conversation

lavigne958
Copy link
Collaborator

@lavigne958 lavigne958 commented Jan 30, 2024 •
edited by alifeee
Loading

  • fix missing methods in http client
    • export()
    • list_permissions()
    • insert_permission()
    • remove_permission()
  • Add test to export methods
  • Fix missing method in HTTP Client

closes #1385

@lavigne958
fix missing method export in http client
306ab7a 
The `HTTPClient` class was missing the `export` method.

Add it in order for the gspread `Client` and the
`Spreadsheet` class to use it to export spreadsheets.

closes #1385

Signed-off-by: Alexandre Lavigne <lavigne958@gmail.com>
@lavigne958
Add test to export methods
5b36a4c 
Add new test that exports a spreadsheet, using
the gspread client and the spreadsheet itself.

We serialize our requests into a JSON file.
JSON cannot serialize binary data (like PDF)
so we can only test with CSV.

Signed-off-by: Alexandre Lavigne <lavigne958@gmail.com>
@lavigne958
Fix missing method in HTTP Client
90fef5f 
Add missing methods to the HTTP client.

update code so the gspread Client and the spreadsheet use
these new methods in order to:
- insert
- list
- remove

a permission.

We can't test these function automatically as this requires
exposing personal information in the recorded requests.

We'll add more tests later when we find a way.

closes #1385

Signed-off-by: Alexandre Lavigne <lavigne958@gmail.com>
@lavigne958 lavigne958 self-assigned this Jan 30, 2024
@lavigne958
Fixup pyupgrade linter apply
1ea9e40 
Signed-off-by: Alexandre Lavigne <lavigne958@gmail.com>
@alifeee
Copy link
Collaborator

alifeee commented Jan 31, 2024

nice change :)

you are doing a relative lot of PRs, I hope you are having enough time for your sanity :)

do we know for sure that these are all the "missing methods"? could there be others? I suppose you have manually checked each client method to see if it would "be missing" or not.

Also, I understand this is a "bandage PR" to get people's code working, but, the methods should not be in the http_client, right? They belong in the gspread Client object, do you think? It seems unreasonable for every custom client to implement all the methods copy, share, etc.

alifeee
alifeee approved these changes Jan 31, 2024
View reviewed changes
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.

I will approve this for the minor patch, but I think we should start an issue to track/discuss whether these methods should be in the http_client or not

@alifeee alifeee added this to the 6.0.1 milestone Jan 31, 2024
@lavigne958
Copy link
Collaborator Author

nice change :)

you are doing a relative lot of PRs, I hope you are having enough time for your sanity :)

Ha ha thanks 😉 I do so far so good.

do we know for sure that these are all the "missing methods"? could there be others? I suppose you have manually checked each client method to see if it would "be missing" or not.

Yes I checked, I noticed we miss some typing in the __init__ method of the Spredsheet object, I added the typing temporarily to identify quickly the missing methods, then removed it so we don't mix unrelated changes.

Also, I understand this is a "bandage PR" to get people's code working, but, the methods should not be in the http_client, right? They belong in the gspread Client object, do you think? It seems unreasonable for every custom client to implement all the methods copy, share, etc.

I agree, this is temporary, though we might keep it until the next major release (could be much closer than the last one), unless we find a way to move it and keep people's code working (as usual 👀 )

I opened the issue #1399 to discuss it.

@lavigne958
Copy link
Collaborator Author

I will approve this for the minor patch, but I think we should start an issue to track/discuss whether these methods should be in the http_client or not

agreed, as mentioned above I created the issue to discuss it.

@lavigne958 lavigne958 merged commit 3c02a2c into master Jan 31, 2024
10 checks passed
@lavigne958 lavigne958 deleted the bugfix/client_export branch January 31, 2024 18:52
@alifeee
Copy link
Collaborator

alifeee commented Jan 31, 2024

I agree, this is temporary, though we might keep it until the next major release (could be much closer than the last one)

my preference would be to make as few major releases as possible, ever. It is not fun to deal with the churn of software

@alifeee
Copy link
Collaborator

alifeee commented Jan 31, 2024

Fix missing method in HTTP Client

I think for when we come back to it, you should list which methods you fixed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alifeee alifeee alifeee approved these changes

Assignees

@lavigne958 lavigne958

Labels
None yet
Projects
None yet
Milestone
6.0.1
Development

Successfully merging this pull request may close these issues.

Client functions in v6.0.0 do not work - HTTP Client Object has no attribute insert_permission
2 participants
@lavigne958 @alifeee