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.

Sign up for GitHub

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

Jump to bottom

New type StrLimited #3724

Closed
wants to merge 7 commits into from
Closed

New type StrLimited #3724

wants to merge 7 commits into from

Conversation

lucastosetto
Copy link

@lucastosetto lucastosetto commented Jan 23, 2022 •
edited

Change Summary

This represents a simplified length limited string and is an alias for the following syntax:

StrLimited[max_length] = constr(max_length=max_length, strip_whitespace=True)

Usage example:

class Foo:
    bar: StrLimited[10] # bar is a str with maximum length of 10 characters

Related issue number

fix #2104

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

@lucastosetto
Copy link
Author

please review

samuelcolvin
samuelcolvin reviewed Apr 2, 2022
View reviewed changes
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, will need to be added to docs.

changes/2104-lucastosetto.md Outdated Show resolved Hide resolved
pydantic/types.py Outdated Show resolved Hide resolved
pydantic/types.py
@@ -436,6 +437,12 @@ def constr(
return _registered(type('ConstrainedStrValue', (ConstrainedStr,), namespace))


class StrLimited:
Copy link
Member

Choose a reason for hiding this comment

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

you might want to implement __get_validators__ and raise an error people try to use StrLimited without parameterising it.

tests/test_types.py Outdated Show resolved Hide resolved
tests/test_types.py Outdated Show resolved Hide resolved
tests/test_types.py Outdated Show resolved Hide resolved
pydantic/types.py Outdated
@@ -436,6 +437,12 @@ def constr(
return _registered(type('ConstrainedStrValue', (ConstrainedStr,), namespace))


class StrLimited:
@classmethod
def __class_getitem__(cls, max_length: int) -> Type[str]:
Copy link
Member

Choose a reason for hiding this comment

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

We could also be clever and allow slice usage:

class MyModel(BaseModel):
    has_max_length: StrLimited[10]
    has_min_and_max_length: StrLimited[5:10]

WDYT?

@samuelcolvin
Copy link
Member

please update.

lucastosetto and others added 5 commits July 3, 2022 17:03
@lucastosetto @samuelcolvin
Update changes/2104-lucastosetto.md
8349c7a 
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@lucastosetto @samuelcolvin
Update pydantic/types.py
a174228 
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@lucastosetto @samuelcolvin
Update tests/test_types.py
88f2e81 
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@lucastosetto @samuelcolvin
Update tests/test_types.py
133a378 
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
@lucastosetto @samuelcolvin
Update tests/test_types.py
422f9f2 
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
samuelcolvin
samuelcolvin requested changes Aug 8, 2022
View reviewed changes
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 other things still to do:

  • __get_validators__ as suggested above
  • add docs, as suggested above

tests/test_types.py
@@ -733,6 +734,34 @@ class Model(BaseModel):
]


def test_str_limited_good():
max_length = 5
Copy link
Member

Choose a reason for hiding this comment

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

you can remove this, it's not used.

tests/test_types.py


def test_str_limited_too_long():
max_length = 5
Copy link
Member

Choose a reason for hiding this comment

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

you can remove this and just use a hard-coded value below, it's not adding anything.

@samuelcolvin
Copy link
Member

@lucastosetto any chance you can fix this?

@samuelcolvin
Copy link
Member

samuelcolvin commented Aug 19, 2022 •
edited

Notice

See twitter 🐦, I've you'd like this to be included in V1.10, please fix it and request a review TODAY.

Or if you need this in V1.10 but don't have time to complete it (or aren't the author), please comment here and on #4324

@lucastosetto
Copy link
Author

@lucastosetto any chance you can fix this?

Sure, I can work on it this week, but I won't have time to do it for v1.10 release.

@samuelcolvin
Copy link
Member

samuelcolvin commented Aug 21, 2022 •
edited

Ok, I might try to finish it for you.

If it doesn't make it into v1.10 much of the implementation will have to change for V2.

@samuelcolvin
Copy link
Member

Ok, I might try to finish it for you.

Ignore that, I'm not going to have time to work on this, I think it'll have to wait for V2.

@samuelcolvin samuelcolvin added deferred deferred until future release or until something else gets done and removed awaiting author revision labels Aug 22, 2022
antonagestam
antonagestam reviewed Aug 28, 2022
View reviewed changes
pydantic/types.py
Comment on lines +441 to +443
@classmethod
def __class_getitem__(cls, max_length: int) -> Type[ConstrainedStr]:
return constr(max_length=max_length, strip_whitespace=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

This usage of __class_getitem__ isn't compatible with static typing, at least mypy will give an error for StrLimited[10], and I believe the other major type checkers would too. Mypy gives: error: The type "Type[StrLimited]" is not generic and not indexable [misc]. I'd suggest redesigning this to make StrLimited an object that implements __get_item__ instead.

@samuelcolvin
Copy link
Member

wasn't fixed and now conflicting, I'd still be happy to accept a fix for #2104 if someone would like to submit a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonagestam antonagestam antonagestam left review comments

@samuelcolvin samuelcolvin samuelcolvin requested changes

Assignees

@lucastosetto lucastosetto

Labels
deferred deferred until future release or until something else gets done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Simplified length limited string
4 participants
@lucastosetto @samuelcolvin @antonagestam @PrettyWood