Skip to content

Conversation

JonAnCla
Copy link
Contributor

@JonAnCla JonAnCla commented Jan 27, 2022

Change Summary

Initial implementation of ConstrainedDate, condate, with tests and docs

  • ConstrainedDate piggybacks off ConstrainedNumberMeta and number_size_validator to implement the gt/ge/lt/le logic

  • Perhaps those should be renamed to hint that they are not just applicable to numbers?

  • if accepted, more doc changes will be needed to highlight that date validation can be constrained

  • also if accepted suggest similar classes for datetime, time may be needed?

Many thanks for great library!

(p.s. I replaced date with datetime.date throughout types.py to avoid inevitable confusion that otherwise arises between datetime module vs datetime class)

Related issue number

fix #3739

Checklist

  • Unit tests for the changes exist
  • [?] Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@JonAnCla
Copy link
Contributor Author

seems like using ConstrainedNumberMeta as metaclass for ConstrainedDate has broken the tests relating to Hypothesis strategies (_hypothesis_plugin.py)

Would appreciate some guidance on whether this approach makes sense and how to add necessary changes to _hypothesis_plugin.py

@samuelcolvin
Copy link
Member

seems like using ConstrainedNumberMeta as metaclass for ConstrainedDate has broken the tests relating to Hypothesis strategies (_hypothesis_plugin.py)

Would appreciate some guidance on whether this approach makes sense and how to add necessary changes to _hypothesis_plugin.py

@Zac-HD any ideas?

Copy link
Contributor

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Suggested fix below, though I haven't tested it. I'd also recommend renaming the class ConstrainedScalarMeta, because a date (or datetime) is definitely not a number.

Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>
@Zac-HD
Copy link
Contributor

Zac-HD commented Apr 4, 2022

Unsubscribing because this looks like standard "read CI logs and fix errors" territory; ping me again if you have a Hypothesis question.

@JonAnCla
Copy link
Contributor Author

Thanks for the help and changes, I've now got initial version working and all tests pass.

Some follow up questions for @samuelcolvin

  • I agree with Zac-HD's suggestion to rename ConstrainedNumberMeta to ConstrainedScalarMeta - but I have not implemented this yet as it'll make more widespread changes. at the moment it is fairly straightforward to review the changes. let me know if you'd like me to go ahead with that rename?
  • more docs changes are needed to highlight that date validation can be constrained. I can add these as long as your are happy with basic implementation
  • should I add similar classes for datetime.datetime, and maybe even datetime.time? might be overkill?

Thanks!

@JonAnCla
Copy link
Contributor Author

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.

Looking good, some things to change.

### Arguments to `condate`
The following arguments are available when using the `condate` type function

- `strict_formats: List[str] = None`: list of datetime.datetime.strpformat style date formats that will be accepted
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this, it's not supported in V2 so it'll be confusing if we add it now in V1.

(the bounds constraints are already implemented in V2).

@@ -1087,32 +1090,78 @@ def to(self, unit: str) -> float:
# ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ DATE TYPES ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

if TYPE_CHECKING:
PastDate = date
FutureDate = date
PastDate = datetime.date
Copy link
Member

Choose a reason for hiding this comment

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

Best to revert these changes.

@@ -0,0 +1,75 @@
from datetime import date, datetime
Copy link
Member

Choose a reason for hiding this comment

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

can you move this into test_datetime_parse.py, We could do with rearranging tests, putting some tests in this file will cause more confusion.

update_not_none(field_schema, exclusiveMinimum=cls.gt, exclusiveMaximum=cls.lt, minimum=cls.ge, maximum=cls.le)

@classmethod
def strict_date_validator(cls, value: Any) -> datetime.date:
Copy link
Member

Choose a reason for hiding this comment

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

as per my above comment, remove this.

@samuelcolvin
Copy link
Member

please update.

@samuelcolvin
Copy link
Member

you'll also need to rebase or merge master to get all a green tick.

@github-actions github-actions bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Aug 8, 2022
@github-actions github-actions bot assigned JonAnCla and unassigned samuelcolvin and PrettyWood Aug 8, 2022
@JonAnCla
Copy link
Contributor Author

Hopefully that covers everything you mentioned :)

Sorry if some of this overlaps with changes already envisioned for V2

And thanks for the great work!

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.

Looking good but a few more things to fix.

@JonAnCla
Copy link
Contributor Author

Hopefully nearly there now :)

@samuelcolvin samuelcolvin changed the title Initial implementation of ConstrainedDate, condate. Implements #3739 Initial implementation of ConstrainedDate, condate Aug 12, 2022
@samuelcolvin samuelcolvin enabled auto-merge (squash) August 12, 2022 14:29
@samuelcolvin
Copy link
Member

this is great, thank you so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting author revision awaiting changes from the PR author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants