-
Notifications
You must be signed in to change notification settings - Fork 815
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
Validation #2600
Validation #2600
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some comments. Will do a review tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Some comments, and maybe-issues.
src/textual/validation.py
Outdated
|
||
def __post_init__(self) -> None: | ||
# If a failure message isn't supplied, try to get it from the Validator. | ||
if self.description is None and self.validator is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.validator
can never be None
according the the type.
I gather this sets description
if its None. But that means that description
is still type as being | None
, which it never will be from the POV of the Textual developer.
Can this logic be moved to where a Failure
is constructed? Thus removing the need to type description
as potentially being None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is possible without having to duplicate this logic across all validators (meaning users writing custom validators would also have to duplicate this logic). I think things will become much messier and the trade-off isn't worth it unless you can suggest another approach.
Is the description being None
bad? Maybe as a developer who wants a custom Validator
, I just have no need to describe my validation failure because I won't be using the description anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some requests re error messages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Here's an example which covers lots of the new additions (video and code below):
Palindrome
)"That's not a palindrome :/"
)-valid
and-invalid
styles applied toInput
. Note that only-invalid
has styling by default.-valid
has no default styling - but we attach it anyway so that users have the flexibility to change styling when validation explicitly passes. In the example below, when validation passes, the border of the Input goes green.validation-23may23.mov