-
Notifications
You must be signed in to change notification settings - Fork 7
Fallback to configured project settings when left unspecified on model and form fields #54
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
base: main
Are you sure you want to change the base?
Conversation
Added tests, back to 100% coverage. This should be ready for review and merge. |
@marksweb This should be ready to merge. |
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.
Had a quick look over this, but need to find some time to go over the rest later. Some first comments though.
I'm wondering if this would be a good place/time to change the field values to indicate what parameters specify allowlists. Something like this: class MyModel(models.Model):
content = Nh3Field(
allowed_tags=["a", "b"],
allowed_attributes={
"*": {"class", "style"},
},
allowed_attribute_filter=lambda element, attribute, attribute_value: ...,
allowed_generic_attribute_prefixes={"my-custom-tag-prefix"},
allowed_tag_attribute_values={
"a": {
"class": {
"bold",
"col-1", "col-2", "col-3",
"col-4", "col-5", "col-6",
"col-7", "col-8", "col-9",
"col-10", "col-11", "col-12",
"emphasized",
},
},
allowed_url_schemes={"gopher", "https"},
) If we're going to make a breaking change to keyword-only arguments, we might as well change the names of the arguments to be absolutely clearer about what they mean. Even being somewhat familiar with Python and Bleach, and reading the Nh3 docs, it still took me awhile to figure out how I was supposed to configure this project to work for me. More descriptive argument names would have made that easier and more clear. Thoughts? |
Yeah I think that makes sense, if it makes sense to you when you're using it, as you say 👍🏼 |
Great! I crafted a separate PR specifically for that then: PR #57. This PR is big enough as it is. And pending your review, this should be good to go. |
This PR builds on top of PR #37 and supersedes it.
This project currently requires users to pass in all NH3 configuration options wherever they use an NH3 model field or form field. Options for model fields and form fields do not follow any project-wide configured settings values.
Instead, pull any unspecified NH3 model field and form field options from the Django project
settings.py
module. Essentially supporting project-wide configuration (viasettings.py
), and per-field configuration. This follows the DRY principle and makes it easier to use Nh3Fields.I also decomposed a few functions in this package's
utils
module a bit more, which then allowed me to simplify them further.Closes #37.