[Django] #36708: `formset` not a static attribute of `ChangeList`

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben | Owner: Ben Gregory
Gregory |
Type: | Status: assigned
Cleanup/optimization |
Component: | Version: 5.2
contrib.admin |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
The `ChangeList` class is used in Django Admin to determine and configure
the functionality of a model's `change_list` admin page.

I have been experimenting with using `ChangeList` for my own purposes, to
create a custom page not dissimilar to `change_list.html`.

I have noted that in doing so, I have needed to set the attribute `formset
= None` on my `ChangeList` instance, to satisfy an expectation of the
`admin_list.result_list`
[https://github.com/django/django/blob/e27cff68a32a0183c6b8d110b359c1c858f68cd7/django/contrib/admin/templatetags/admin_list.py#L328
template tag]. This is because `formset` is not a static attribute of the
class.

Looking at the source code, the
[https://github.com/django/django/blob/e27cff68a32a0183c6b8d110b359c1c858f68cd7/django/contrib/admin/options.py#L2056
`django.contrib.admin.options`] module sets `formset` on the `ChangeList`
instance.

Outside of tests, `ChangeList` objects are only instantiated in the above
module, and as said above, will always be assigned a `formset` attribute.
See
[https://github.com/search?q=repo%3Adjango%2Fdjango+%2F%28%3F-i%29ChangeList%2F+language%3APython&type=code&l=Python
here]. So, it appears to me that there is an assumption that instances of
this class should _always_ have a `formset` attribute.

I propose we simply make `formset` an attribute of `ChangeList`, rather
than assign it dynamically in the aforementioned function. This will mean
developers working low-level in the admin needn't write a line of code to
be able to use their custom `ChangeList` objects with the relevant
template tags, and will serve to better document the expected attributes
of `ChangeList` objects more generally.

Proposal:

https://github.com/django/django/blob/e27cff68a32a0183c6b8d110b359c1c858f68cd7/django/contrib/admin/views/main.py#L67:

{{{
+ formset = None
}}}

https://github.com/django/django/blob/e27cff68a32a0183c6b8d110b359c1c858f68cd7/django/contrib/admin/options.py#L2056:

{{{
- formset = cl.formset = None
+ formset = None
}}}

One could also argue that this is an opportunity to remove this notation
`formset = cl.formset = *`, and use `cl.formset` directly.
--
Ticket URL: <https://code.djangoproject.com/ticket/36708>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------

Reporter: Ben Gregory | Owner: Ben
Type: | Gregory
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: 5.2
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Antoliny):

* cc: Antoliny (added)
* keywords: => ChangeList

Comment:

Thank you for suggesting the improvements Ben!

You're absolutely right, when the `result_list` template tag renders the
template, the changelist must have a `formset` attribute.
{{{
def results(cl):
if cl.formset:
for res, form in zip(cl.result_list, cl.formset.forms):
yield ResultList(form, items_for_result(cl, res, form))
else:
for res in cl.result_list:
yield ResultList(None, items_for_result(cl, res, None))

def result_list(cl):
"""
Display the headers and data list together.
"""
headers = list(result_headers(cl))
num_sorted_fields = 0
for h in headers:
if h["sortable"] and h["sorted"]:
num_sorted_fields += 1
return {
"cl": cl,
"result_hidden_fields": list(result_hidden_fields(cl)),
"result_headers": headers,
"num_sorted_fields": num_sorted_fields,
"results": list(results(cl)),
}
}}}
And as you mentioned, this attribute is currently being dynamically
assigned in the `changelist_view` through the options.py.

I also agree that defining the formset attribute directly in the
`ChangeList` class would be more appropriate.
I agree with this improvement, but there might be aspects I haven’t
considered, so I’ll wait for another person’s triage :)
--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:1>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Ben
Type: | Gregory
Cleanup/optimization | Status: assigned

Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* easy: 0 => 1
* stage: Unreviewed => Accepted
* version: 5.2 => dev

Comment:

Thanks for the report. And thanks Antoliny for the triage, I trust your
view on admin issues :-)

Looks like this may be the issue from #13039, but I'm happy to keep this
issue open instead of reopening there.

Ben, would you like to submit a PR?
--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:2>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Ben
Type: | Gregory
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rudraksha Dwivedi):

PR submitted: https://github.com/django/django/pull/20184
I think it addresses the issue aptly, I shall set the Has patch to yes
--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:3>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Ben
Type: | Gregory
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Rudraksha Dwivedi):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:4>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------

Reporter: Ben Gregory | Owner: Rudraksha
Type: | Dwivedi

Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Antoliny):

* owner: Ben Gregory => Rudraksha Dwivedi

--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:5>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Rudraksha
Type: | Dwivedi
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Antoliny):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:6>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Rudraksha
Type: | Dwivedi
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rudraksha Dwivedi):

can someone shed some light on the flake8 test on the automated github
testing flow ?
i think the issue is solved but some linting test fail
so i am not setting needs improvement to no currently
thank you!
--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:7>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Rudraksha
Type: | Dwivedi
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Antoliny):

Replying to [comment:7 Rudraksha Dwivedi]:

> can someone shed some light on the flake8 test on the automated github
testing flow ?
> i think the issue is solved but some linting test fail
> so i am not setting needs improvement to no currently
> thank you!

Would you like to refer to this
[https://docs.djangoproject.com/en/5.2/internals/contributing/writing-code
/unit-tests/#running-tests-using-to document]?

You can run the tests with tox as well, but if you use pre-commit, lint
tests will run automatically when you make a commit!
--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:8>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Rudraksha
Type: | Dwivedi
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rudraksha Dwivedi):

Replying to [comment:8 Antoliny]:

> Replying to [comment:7 Rudraksha Dwivedi]:
> > can someone shed some light on the flake8 test on the automated github
testing flow ?
> > i think the issue is solved but some linting test fail
> > so i am not setting needs improvement to no currently
> > thank you!
>
> Would you like to refer to this
[https://docs.djangoproject.com/en/5.2/internals/contributing/writing-code
/unit-tests/#running-tests-using-to document]?
>
> You can run the tests with tox as well, but if you use pre-commit, lint
tests will run automatically when you make a commit!

hey thanks for your response!!
i read through the article and your suggestion about about precommit lint
test was really helpful
however due to some odd reason even after running the precommit test
with relevant test log = (
black....................................................................Passed
blacken-docs.........................................(no files to
check)Skipped
isort....................................................................Passed
flake8...................................................................Passed
eslint...............................................(no files to
check)Skipped
[ticket-36708 e877e5a3be] Cleanup for flake8 check
1 file changed, 1 insertion(+), 1 deletion(-)
)

to me it looked like its done but after pushing the GitHub automated flow
showed same issue can i get some more cues on what i can do ?
also, unrelated question is committing + pushing to the PR so many times
(to fix the issue of this nature where u can only see the result on
pushing) considered a bad practise ?
thanks !
--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:9>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Rudraksha
Type: | Dwivedi
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Antoliny):

Running only flake8 locally through tox is also an option.

{{{
tox -e flake8
}}}

Replying to [comment:9 Rudraksha Dwivedi]:
> Since everything appeared clean locally, I expected the GitHub workflow
to pass as well, but the same flake8 issue is still reported in CI. Could
you provide any guidance on what else I should check or what might cause
this discrepancy?
> Also, on a related note: is it generally acceptable to make several
small commits and pushes to a PR when iterating on CI failures, or is this
discouraged?
> Thanks again.

I’m also not entirely sure how to answer this part.
It probably won’t be a big issue, but once you get more familiar with
Django’s CI, I think you won’t have to worry much about these CI-related
concerns :)
--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:10>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Rudraksha
Type: | Dwivedi
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rudraksha Dwivedi):

Replying to [comment:10 Antoliny]:

> Running only flake8 locally through tox is also an option.
>
> {{{
> tox -e flake8
> }}}
>
> Replying to [comment:9 Rudraksha Dwivedi]:
> > Since everything appeared clean locally, I expected the GitHub
workflow to pass as well, but the same flake8 issue is still reported in
CI. Could you provide any guidance on what else I should check or what
might cause this discrepancy?
> > Also, on a related note: is it generally acceptable to make several
small commits and pushes to a PR when iterating on CI failures, or is this
discouraged?
> > Thanks again.
>
> I’m also not entirely sure how to answer this part.
> It probably won’t be a big issue, but once you get more familiar with
Django’s CI, I think you won’t have to worry much about these CI-related
concerns :)

hey! Thanks for being patient ! i realised how i had duplicated the same
like of code in two distinct places, also figured out how to setup and
test locally properly
Thanks ^_____^
If the issues dont occur i will mark needs improvement as no
peace ✨
--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:11>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Rudraksha
Type: | Dwivedi
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Rudraksha Dwivedi):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:12>

[Django]

#36708: `formset` not a static attribute of `ChangeList`
-------------------------------------+-------------------------------------
Reporter: Ben Gregory | Owner: Rudraksha
Type: | Dwivedi
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: ChangeList | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36708#comment:13>