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

Added 'add_data_validation to Workhsheet` [Issue #1420] #1444

Merged
merged 7 commits into from
Apr 5, 2024

Conversation

muddi900
Copy link
Contributor

@muddi900 muddi900 commented Mar 21, 2024

Added a RealtiveDate enum

removed the realtive date enum

Closes #1420

@alifeee
Copy link
Collaborator

alifeee commented Mar 21, 2024

hi :) thanks for the PR

it looks like you have to run tox -e format locally to format the code, so the linter is happy.

after that, the CI should pass, and we will be able to review this change :)

@alifeee
Copy link
Collaborator

alifeee commented Mar 21, 2024

Hi, that looks good. Sorry there keep being issues.

Try deleting the cassette, and re-running the test to generate it again. Sometimes there are conflicts.

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.

Hi thank you for this PR, I need to test it and check the API to confirm.

gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/utils.py Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
@lavigne958
Copy link
Collaborator

What type of cassette recording mode did you use ?

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 proposal. it's well made.

You do need to change the typing notation in order to pass the CI linter.

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

Please follow the given indications so the test + documentation build can succeed.

Could you please rename the argument showCustomUi to showCustomUI with a uppercase i, it's an acronym so it should be uppercase.

after that it's all good to, we will merge it once the changes are applied.

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

I checked and the tests won't pass until you remove the following file and record it again.
tests/cassettes/WorksheetTest.test_add_validation.json

Quick trick, in order to regenerate only this cassette you can use the following command:
tox -e py -- -k test_add_validation
don't forget to set the env variables GS_CREDS_FILENAME and GS_RECORD_MODE=new_episodes so it only records this missing cassette.

@lavigne958
Copy link
Collaborator

Great thanks. Last thing left is the documentation to fix then all tests should pass.

@muddi900
Copy link
Contributor Author

Seems all tests are working, I'll flatten the commits in a moment

lavigne958
lavigne958 previously approved these changes Mar 29, 2024
gspread/worksheet.py Outdated Show resolved Hide resolved
@lavigne958 lavigne958 dismissed their stale review March 29, 2024 13:28

missed wrong example

@alifeee
Copy link
Collaborator

alifeee commented Mar 29, 2024

this looks wonderful

would we like to put it in any docs like the main rst example file, or the README, so that people actually know this new function exists?

:)

@muddi900
Copy link
Contributor Author

muddi900 commented Mar 29, 2024 via email

@alifeee
Copy link
Collaborator

alifeee commented Mar 29, 2024

hmm, not sure about markdown to rst...

but I would add a simple code snippet, like

ws.add_validation(
                'C2:C7',
                ValidationConditionType.one_of_list,
                ['Yes','No'],
                showCustomUi=True
            )

to the README (1)...

https://github.com/burnash/gspread/blob/master/README.md?plain=1

...and the user guide (2)...

https://github.com/burnash/gspread/blob/master/docs/user-guide.rst?plain=1

... in an appropriate place. For (1), it's markdown. For (2), you can run tox -e doc to build the docs to see if they build properly.

tests/worksheet_test.py Outdated Show resolved Hide resolved
added a generic request.

finsihed the method

conditon type input uses enum from utils

Added a some docs and crave-out for relative dates

Completed the docs.

fixed the position for kwargs

Adding return value

Added a test for `add_validation`

added validation condtion enum

Added a RealtiveDate enum

removed the realtive date enum

fixed format errors

Explicit kwargs for optional params

Removed reference RelativeDate

Added a type check for `condition_type`.

added examples

fixed formatting for docs

Unbounded values to a standard iterable.

Switched typing syntax to be `Optional` for compatibility.

renamed range and  removed blank args.

re ran the test

fixed the doc issue
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.

great addon with the documentation.

please remove unwanted changes and we should be good to go

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@lavigne958
Copy link
Collaborator

great work thank you for this contribution !

@lavigne958 lavigne958 merged commit fb065dc into burnash:master Apr 5, 2024
5 checks passed
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.

A way to add DataValidation
3 participants