#33990 closed Cleanup/optimization (fixed)
Inconsistent capitalization of Set in assertion functions
Reported by: | John Litborn | Owned by: | Michael Howitz |
---|---|---|---|
Component: | Testing framework | Version: | 4.1 |
Severity: | Normal | Keywords: | naming, capitalization, camelcase |
Cc: | David Sanders | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
FormSet and QuerySet for some reason doesn't have a capitalized S in assertFormsetError
and assertQuerysetEqual
, whereas all other instances of camel-cased names in the codebase containing either of them have capitalized S's: (InlineAdminFormSet, Base[...]FormSet, EmptyQuerySet, RawQuerySet).
Looking at the other assertion methods inherited from unittest.TestCase
they're incredibly cased (e.g. assertMultiLineEqual
) so I'm not finding any convention of not uppercasing liberally.
My suggestion is to rename assertFormsetError
->assertFormSetError
and assertQuerysetEqual
->assertQuerySetEqual
in django.test.TestCase
and add aliases for the old names so as not to break existing code. In the code base this is a quite small change, but there's likely some docs (and e.g. the tutorial) that might warrant updating to new spelling. Could also (down the line) add a deprecationwarning to get rid of the old spelling if you want a cleaner API interface.
This bit me as I was following the tutorial and re-typing the code instead of copy-pasting, so it's plausible that others might encounter it similarly if they don't have an IDE that suggests the alternate spelling.
Change History (17)
comment:1 by , 2 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:2 by , 2 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Cleanup/optimization |
comment:3 by , 2 years ago
Owner: | changed from | to
---|
Taking over the assignment as I am on a sprint and able to work on it.
comment:4 by , 2 years ago
Has patch: | set |
---|
comment:5 by , 2 years ago
Just checking the docs to see how things are named there compared to the code: Looks like QuerySet is capitalised as per code but "Formset" tends to use classic sentence casing:
ie see how QuerySet is used in a sentence: https://docs.djangoproject.com/en/4.1/ref/models/querysets/
compared to how "formset" is used in a sentence: https://docs.djangoproject.com/en/4.1/topics/forms/formsets/
comment:6 by , 2 years ago
Cc: | added |
---|
comment:7 by , 2 years ago
The issue in django-upgrade
is https://github.com/adamchainz/django-upgrade/issues/252.
comment:8 by , 2 years ago
The PR in django-upgrade
is https://github.com/adamchainz/django-upgrade/pull/253.
comment:10 by , 2 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:12 by , 2 years ago
Patch needs improvement: | set |
---|
comment:13 by , 2 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:15 by , 2 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
OK — I'm not 100% convinced this is worth the change, so if someone wants to mark it
wontfix
I won't complain, but… — let's accept as a cleanup for theassertFormsetError
andassertQuerysetEqual
methods.