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

Non boolean values for field_flags #467

Merged
merged 13 commits into from
Jun 14, 2020
Merged

Conversation

Sohalt
Copy link
Contributor

@Sohalt Sohalt commented Feb 12, 2019

This allows to specify a function returning a dictionary for field_flags.
This allows validators to set flags which are supported directly in HTML for client side validation, such as minlength, which then get rendered appropriately by the widgets supporting that attribute.

Fixes #406

@Sohalt
Copy link
Contributor Author

Sohalt commented Feb 12, 2019

I haven't written any documentation yet. Also this could probably use some more tests. And ideally the Regexp validator would set the pattern attribute, but I am not sure if you can just set the Python pattern verbatim, because JS might support a different subset of regex.

@Sohalt
Copy link
Contributor Author

Sohalt commented Feb 18, 2019

@davidism the stylecheck fails because some lines are too long (> 80 characters). The docs say this is not a hard limit and I see other lines in test_fields.py with more than 80 characters that don't get flagged. Can you please have a look and tell me how to best resolve this? The affected lines are html literals for testing, for which it doesn't make much sense to split them over multiple lines imho.

@davidism
Copy link
Member

I believe Flake8 is configure to allow what Black allows: 88 characters max. Literals should be split with implicit continuations:

value = (
    "This is a"
    " long sentance."
)

tests/test_fields.py Outdated Show resolved Hide resolved
@Sohalt
Copy link
Contributor Author

Sohalt commented Feb 18, 2019

For some reason coveralls fails. I don't think this is an issue with my code.

@Sohalt Sohalt closed this Feb 20, 2019
@Sohalt Sohalt reopened this Feb 20, 2019
@Sohalt
Copy link
Contributor Author

Sohalt commented Feb 20, 2019

Ok, it seems the coveralls API failed. Simply restarting the job worked… Can you have a look at the code now @davidism and tell me any issues? If everything is ok code wise, I'll write some documentation.

@davidism
Copy link
Member

When we have time, I or another maintainer will handle this.

@wtforms wtforms deleted a comment from Sohalt May 10, 2019
@wtforms wtforms deleted a comment from Sohalt May 10, 2019
@Sohalt
Copy link
Contributor Author

Sohalt commented May 10, 2019

Thank you! I just wanted to make sure this is still on your radar.

@whb07
Copy link
Contributor

whb07 commented Jul 21, 2019

@Sohalt I've been afk for a bit on this project 😞, so could you tell me why you chose to go with a function call rather than a property/attribute of sorts?

@Sohalt
Copy link
Contributor Author

Sohalt commented Jul 23, 2019

It's been a while, so I don't exactly remember the reason. But I think if I remember correctly I originally tried implementing it with a dictionary, which failed however for my specific use case. I think using the lambda was required to delay evaluation, because some information was not yet available when setting the flags.

@azmeuk azmeuk added the enhancement New feature, or existing feature improvement label Apr 21, 2020
@azmeuk
Copy link
Member

azmeuk commented Apr 21, 2020

I switched the tests to the pytest syntax and rebased this branch.

I can understand the use of field_flags being a function, to dynamically create a dict, though this is not the only way to do that.
I think I would like it better if field_flags was a dict initialized in validators __init__.

Then with some documentation I think this PR would look good.

@azmeuk azmeuk force-pushed the field-flags branch 2 times, most recently from 26acfbf to d550f05 Compare April 23, 2020 15:48
@azmeuk
Copy link
Member

azmeuk commented Apr 23, 2020

This seems good to me now. However I don't know if this patch should target v3.x or v2.x.
Since it does not break anything, I think both are possible.

if isinstance(flags, tuple):
warnings.warn(
"Flags should be stored in dicts and not in tuples. "
"The next version of WTForms will abandon support "
Copy link
Member

Choose a reason for hiding this comment

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

Depending on which branch this PR lands, we should explicit the next version.

@davidism davidism added this to the 3.0 milestone Apr 24, 2020
@azmeuk azmeuk requested a review from davidism May 28, 2020 21:06
@azmeuk azmeuk mentioned this pull request May 31, 2020
@azmeuk azmeuk merged commit f679e3c into wtforms:master Jun 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, or existing feature improvement
Development

Successfully merging this pull request may close these issues.

Allow non-boolean field flags
4 participants