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

Rebase on stricter types for fields #14

Merged
merged 10 commits into from
Apr 18, 2024

Conversation

roll
Copy link
Member

@roll roll commented Apr 17, 2024


Please make sure that all the checks pass. Please add here any additional information regarding this pull request. It's highly recommended that you link this PR to an issue (please create one if it doesn't exist for this PR)

@roll
Copy link
Member Author

roll commented Apr 17, 2024

@khusmann
If you have some time, can you please take a look? In general, it seems to be just working. I didn't run into any problems in the implementation

@khusmann
Copy link

khusmann commented Apr 17, 2024

BEAUTIFUL! This is so nice to use:

image

In addition to the code completion, the types here really help with mapping / translating between formats (and converting all the fields properly) because I can guarantee I've handled all the possible cases via exhaustiveness checking (less unit tests needed to catch runtime errors!). Thank you so much!


Small suggestion -- I think you should make the BaseConstraints and ValueConstraints generic types, (which is supported by pydantic) rather than using Any in their properties.

This way, properties of constraints can return the correct types. For example, IntegerField.constraints would have type ValueConstraints[int]. Then, when I use constraints.enum, constraints.minimum or constraints.maximum, they will all return int types instead of Any .

Related: I've proposed a dialect.type field so we can do similar magic on the different possible types of dialect: frictionlessdata/datapackage#921

@roll
Copy link
Member Author

roll commented Apr 18, 2024

@khusmann
Thaks for the review!

Small suggestion -- I think you should make the BaseConstraints and ValueConstraints generic types, (which is supported by pydantic) rather than using Any in their properties.

Good catch! Added generics now

@roll roll merged commit c23eddf into main Apr 18, 2024
9 checks passed
@roll roll deleted the 11/rebase-on-stricter-types-for-fields branch April 18, 2024 08:20
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.

Rebase on stricter types for fields?
2 participants