[Django] #22137: Widgets class should drop the is_hidden bool property
Django
-------------------------------+--------------------
Reporter: django@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
To me, it seems somewhat unnecessary, here are the reasons why:
* In every place that `is_hidden = True` is set in `forms/widgets.py`,
there is also a corresponding `input_type = 'hidden'` set. It seems like
we should use one ''or'' the other for determining if a field is hidden
(keep it DRY)
* `is_hidden`doesn't necessarily reflect whether the widget is actually an
input of type `type='hidden'` or not
* `is_hidden` cannot be set in the `__init__()` method, whereas
`input_type` can (by passing an `attrs={'type' : 'hidden' }` dict)
The scenario where I came across this was that I wanted to make a
`DateInput` widget hidden in one of my `ModelForms`. A basic example:
{{{
# Model
class MyMod(models.Model):
date = models.DateField()
# Form
class MyModForm(forms.ModelForm):
class Meta:
widgets = {
'date' : forms.DateInput(format="%d/%m/%Y",
attrs={'type' : 'hidden'}) # I need to use a DateInput here because a
HiddenInput does not format the date properly (this is probably another
bug in Django - the default date format is the US %m/%d/%y and it seems
very difficult to change)
}
# Template
{% if field.field.widget.is_hidden %}
{{ field }}
{% else %}
{{ field.label }} // custom html stuff {{ field }}
}}}
The call in the template does not work, since the DateInput has `is_hidden
= True`. What I have to do in my template is:
`{% if field.field.widget.is_hidden or field.field.widget.input_type ==
"hidden" %}`
Not quite as elegant. If `is_hidden` were to be a method, which would
return something like:
{{{
def is_hidden(self):
return self.input_type == "hidden"
}}}
I think it would be a more reliable way of determining if a widget is
hidden or not.
--
Ticket URL: <https://code.djangoproject.com/ticket/22137>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: django@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anonymous):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Sorry a typo. I should have said (4 lines from the end)
> The call in the template does not work, since the DateInput has
`is_hidden = False`, whereas in fact the DateInput will be hidden in HTML.
What I have to do in my template is:
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:1>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* version: 1.6 => master
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:2>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* has_patch: 0 => 1
Comment:
Attached patch passes tests. Opinion?
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:3>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by alasdair):
Removing is_hidden would be backwards incompatible, so would have to go
through the deprecation cycle. I like the approach of turning it into a
property.
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:4>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
Backwards incompatibility does not automatically means a deprecation
period, we might also simply document it in the release notes, depending
on the severity.
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:5>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by alasdair):
is_hidden is [https://docs.djangoproject.com/en/1.6/topics/forms
/#customizing-the-form-template documented]. My understanding of the
[https://docs.djangoproject.com/en/1.6/misc/api-stability/ api stability
promise] is that we wouldn't remove it without a deprecation cycle unless
there was a good reason.
My projects use `{{ field.is_hidden }}`, so I would have to update my
templates if we removed it. I'm -1 on removing without deprecation
warnings.
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:6>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by alasdair):
* cc: alasdair@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:7>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by django@…):
My thought was to create a property as I said in the original ticket.
{{{
@cached_property
def is_hidden(self):
return self.input_type == "hidden"
}}}
Since it's most likely to be used in templates, it won't make a difference
even if it's not a property. Plus it would fail quietly if you were to
remove it completely without deprecation warnings.
The patch looks good to me: backwards compatible, and does what I want ;-)
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:8>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by alasdair):
claudep's patch looks good to me, sorry if I wasn't clear before.
I saw "drop the is_hidden bool property" in the title, and thought you
were suggesting "remove the attribute or turn it into a property". Re-
reading the ticket, I can see that your suggested solution was always to
create a method/property.
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:9>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
Yes, but there's still a minor backwards incompatibility in that my patch
doesn't allow the property to be set any longer. Thinking about it, we
could even add a property setter so a deprecation warning is raised when
some code tries to set the property (which was previously possible). Note
also that we don't touch at all `BoundField.is_hidden`, but only
`Widget.is_hidden`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:10>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
PR: https://github.com/django/django/pull/2372
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:11>
Django
Reporter: django@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by django@…):
Thanks claudep :)
Really looking forward to Django 1.7. Especially built in migrations!
Thanks everyone for your awesome work.
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:12>
Django
Reporter: django@… | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:13>
Django
Reporter: django@… | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Forms | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz <claude@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"a19f0d0c1e128634b9e393c52148167bf8718b4c"]:
{{{
#!CommitTicketReference repository=""
revision="a19f0d0c1e128634b9e393c52148167bf8718b4c"
Fixed #22137 -- Made Widget.is_hidden a read-only property
Thanks django at patjack.co.uk for the report and the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:14>
Django
Reporter: django@… | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Forms | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"306283bf358470b7d439822af90051ac62e95bae"]:
{{{
#!CommitTicketReference repository=""
revision="306283bf358470b7d439822af90051ac62e95bae"
Removed warning for Widget.is_hidden property.
refs #22137.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22137#comment:15>