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

Add PastDatetime and FutureDatetime types #5720

Merged
merged 4 commits into from
May 10, 2023
Merged

Conversation

hramezani
Copy link
Member

@hramezani hramezani commented May 9, 2023

Error documentation will come later with the validation error docs PR

Selected Reviewer: @samuelcolvin

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 9, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9e88799
Status: ✅  Deploy successful!
Preview URL: https://b9c8cae7.pydantic-docs2.pages.dev
Branch Preview URL: https://future-past-datetime.pydantic-docs2.pages.dev

View logs

@hramezani
Copy link
Member Author

Please review

return core_schema.datetime_schema(now_op='past')
else:
schema = handler(source)
assert schema['type'] == 'datetime'
Copy link
Contributor

@dmontagu dmontagu May 9, 2023

Choose a reason for hiding this comment

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

What is the purpose of this assertion? If there's a chance it could be hit, it may be worth giving a clearer message of what is going wrong if it does get hit. (Same for FutureDatetime below)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I don't know how to hit this line. I just keep it because it was in the other types(AwareDatetime, FutureDate, ...).
@samuelcolvin Do you have any idea here?

Copy link
Member

Choose a reason for hiding this comment

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

I guess the assert would fail if you use it as an annotation on something other than datetime.

If I'm wrong, we can remove, otherwise, yes a better error would be good.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a check that raises an error. added doc and test for error

Copy link
Contributor

@dmontagu dmontagu left a comment

Choose a reason for hiding this comment

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

Made some suggestions above; feel free to take or leave

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise LGTM.

please update.

@@ -23,6 +23,12 @@ types:
`FutureDate`
: like `date`, but the date should be in the future

`PastDatetime`
Copy link
Member

Choose a reason for hiding this comment

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

surely we shouldn't have these documented in two places?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed from here

pydantic/types.py Show resolved Hide resolved
return core_schema.datetime_schema(now_op='past')
else:
schema = handler(source)
assert schema['type'] == 'datetime'
Copy link
Member

Choose a reason for hiding this comment

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

I guess the assert would fail if you use it as an annotation on something other than datetime.

If I'm wrong, we can remove, otherwise, yes a better error would be good.

@hramezani
Copy link
Member Author

@samuelcolvin I've addressed all the comments.

Please review

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

otherwise lGTM.

docs/usage/errors.md Outdated Show resolved Hide resolved
@samuelcolvin samuelcolvin enabled auto-merge (squash) May 10, 2023 15:08
@samuelcolvin samuelcolvin merged commit af9f579 into main May 10, 2023
41 checks passed
@samuelcolvin samuelcolvin deleted the future_past_datetime branch May 10, 2023 15:13
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.

None yet

3 participants