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

Fix Worksheet ID Type Inconsistencies #1269

Merged
merged 9 commits into from
Aug 13, 2023

Conversation

FlantasticDan
Copy link
Contributor

@FlantasticDan FlantasticDan commented Aug 8, 2023

Implements #1267

I'm not super familiar with VCR tests so I hope adding allow_playback_repeats=True is acceptable in this case.

closes #1267

@alifeee
Copy link
Collaborator

alifeee commented Aug 9, 2023

thanks for the PR! Great work :)

Some automated tests fail because of old Python versions. You need to replace int | str with Union[int, str] and from typing import Union (| was added Python 3.10, so earlier versions fail)

For the tests, try this:

  • add your credentials to tests/creds.json
  • run in the console
    GS_CREDS_FILENAME="tests/creds.json"
    GS_RECORD_MODE="all"
    tox -e py -- -k test_get_worksheet_by_id -v -s

This will run the test online (with real API requests).

When you are happy - or the test passes - either clear the env variables or start a fresh console and run the test command(s) again. Let me know if you need any more help with testing.

Again thanks for the PR! I have left some other comments as code comments

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.

thanks for the changes :)

it is great that you write tests

gspread/spreadsheet.py Outdated Show resolved Hide resolved
gspread/spreadsheet.py Outdated Show resolved Hide resolved
gspread/spreadsheet.py Outdated Show resolved Hide resolved
gspread/spreadsheet.py Outdated Show resolved Hide resolved
gspread/spreadsheet.py Outdated Show resolved Hide resolved
tests/spreadsheet_test.py Outdated Show resolved Hide resolved
tests/spreadsheet_test.py Show resolved Hide resolved
tests/spreadsheet_test.py Show resolved Hide resolved
@alifeee alifeee marked this pull request as draft August 9, 2023 14:35
@alifeee alifeee added this to the 5.11.0 milestone Aug 9, 2023
@alifeee
Copy link
Collaborator

alifeee commented Aug 9, 2023

This looks good. You have a merge conflict with your the cassettes. I would perhaps just try deleting the test cassette (json file) and re-running the tests, rather than trying to fix it manually.

After that's fixed, I will have another look :)

@FlantasticDan
Copy link
Contributor Author

So I deleted the .json's, re-ran tests, and pushed the commit with the new ones and now it looks like there's still merge commits but they're subtly different... It seems to me like the conflicts are just the headers coming in a different order in the file than the existing versions which maybe doesn't matter?

@alifeee
Copy link
Collaborator

alifeee commented Aug 9, 2023

the merge conflict was my fault haha. I merged a seperate PR earlier today, but after you opened this.

I have fixed it for you. It all looks good now!

@FlantasticDan
Copy link
Contributor Author

good timing lol, I was just about to go through and do some copy paste hijinks to resolve it.

@alifeee alifeee marked this pull request as ready for review August 9, 2023 19:30
@alifeee alifeee requested a review from lavigne958 August 9, 2023 19:36
@alifeee
Copy link
Collaborator

alifeee commented Aug 9, 2023

thanks so much for this PR. It should be implemented in the next version (v10) in a few weeks.

I consider it good, so once @lavigne958 reviews this we will merge it.

@FlantasticDan
Copy link
Contributor Author

Awesome, thanks for your help @alifeee

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 👍

@lavigne958 lavigne958 merged commit 900e360 into burnash:master Aug 13, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Worksheet ID Type Inconsistencies
3 participants