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

Consider adding allow_empty or allow_blank argument to check_email() #11

Closed
thohan88 opened this issue Jan 9, 2025 · 3 comments
Closed

Comments

@thohan88
Copy link

thohan88 commented Jan 9, 2025

Thank you for this great package!

I noticed that check_email() currently has an allow_none argument, but I'm not sure how useful it is in practice. When using input.text() for email inputs, an empty input returns an empty string (""), not None. This means that even with allow_none=True, the validator won't allow blank emails. For example:

from shiny import App, ui, reactive
from shiny_validate import check, InputValidator

app_ui = ui.page_auto(
    ui.h3("Check e-mail"),
    ui.input_text(id="email", label="E-mail"),
    ui.input_action_button("check_email", "Check e-mail"),
)


def server(input, output, session):

    iv = InputValidator()
    iv.add_rule("email", check.email(allow_none=True))
    iv.enable()


app = App(app_ui, server, debug=True)

shinylive link

Suggestion
Altering or renaming allow_none would introduce breaking changes, even though it seems unlikely that users actually rely on this argument for email validation unless they're using check_email() for other purposes where None values are actually possible. Still, it may be preferable to add a new argument, such as allow_empty or allow_blank, to handle empty strings specifically.

Happy to submit a PR.

@gadenbuie
Copy link
Contributor

I think allow_none is an R -> Python translation of allow_na (in R). We should probably keep it for API stability, so I'm entirely in favor of adding a new argument.

I like allow_empty, but there is some ambiguity – does it mean an empty string or an empty list/tuple/dict? Maybe the distinction isn't important, but the presence of allow_multiple makes it a little confusing. OTOH, allow_empty_string seems overly verbose, so I'd be okay with allow_empty.

That said, for both check.email() and check.url(), it seems that we could just implement a minimum character length before the check kicks in. I'd personally rather separate concerns and have check.required() handle the empty string case and have check.url() start only when the value is present. (Which relates to your other issue #13)

@gadenbuie
Copy link
Contributor

After #14, this issue kind of resolves itself if you use

iv.add_rule("email", check.required())
iv.add_rule("email", check.email())

@thohan88
Copy link
Author

thohan88 commented Jan 9, 2025

Thanks for the very quick fix. You are rocking it, @gadenbuie !

@thohan88 thohan88 closed this as completed Jan 9, 2025
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

No branches or pull requests

2 participants