Opened 7 months ago

Closed 5 months ago

#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 (15)

comment:1 Changed 6 months ago by Anvansh Singh

Owner: changed from nobody to Anvansh Singh
Status: newassigned

comment:2 Changed 6 months ago by Carlton Gibson

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

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 the assertFormsetError and assertQuerysetEqual methods.

  • Uses in Django will need to be updated.
  • The old names will need to be deprecated. (Let's not have two versions floating around...)
  • A matching Issue/PR should be created on The django-upgrade project when this goes in.

comment:3 Changed 6 months ago by Michael Howitz

Owner: changed from Anvansh Singh to Michael Howitz

Taking over the assignment as I am on a sprint and able to work on it.

comment:4 Changed 6 months ago by Michael Howitz

Has patch: set

comment:5 Changed 6 months ago by David Sanders

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 Changed 6 months ago by David Sanders

Cc: David Sanders added

comment:7 Changed 6 months ago by Michael Howitz

comment:8 Changed 6 months ago by Michael Howitz

comment:9 Changed 6 months ago by Mariusz Felisiak

Needs documentation: set
Patch needs improvement: set

comment:10 Changed 6 months ago by Michael Howitz

Needs documentation: unset
Patch needs improvement: unset

comment:11 Changed 6 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 564b317f:

Refs #33990 -- Renamed SimpleTestCase.assertFormsetError() to assertFormSetError().

Co-Authored-By: Michael Howitz <mh@…>

comment:12 Changed 6 months ago by Mariusz Felisiak

Patch needs improvement: set

comment:13 Changed 5 months ago by Mariusz Felisiak

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:14 Changed 5 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In f0c06f8a:

Refs #33990 -- Renamed TransactionTestCase.assertQuerysetEqual() to assertQuerySetEqual().

Co-Authored-By: Michael Howitz <mh@…>

comment:15 Changed 5 months ago by Mariusz Felisiak

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top