Preferred use of SimpleTestCase.settings() and SimpleTestCase.modify_settings().

[ngnpope]

Following on from #17673, I noticed that we could make more use of SimpleTestCase.settings() and SimpleTestCase.modify_settings(). By making them static methods, we can also use them in .setUpClass(), etc. where appropriate.

[ngnpope]

(I was torn by the first commit. It's very straightforward, but is documented, so I guess we should do a ticket?)

[felixxm]

@ngnpope Thanks 👍 It's hard for me to understand why it's worth changing. SimpleTestCase.settings is only a shortcut for override_settings, and override_settings/modify_settings can already be used in setUpClass() and enterClassContext().

[ngnpope]

SimpleTestCase.settings is only a shortcut for override_settings...

Indeed, but it is also presented earlier in the documentation than the global decorator function and caught me out because it didn't work in a class-level scope as I naively assumed it might.

...and override_settings/modify_settings can already be used in setUpClass() and enterClassContext().

Also true. The advantage of the shortcut is that it avoids the need to import the decorator function in some cases.

[nessita]

I've reviewed this work and I agree that having settings and modify_settings being static methods is both more correct and more handy. So from that POV I would certainly merge the first commit. I'm a bit reluctant about commits 2 and 3, considering our rule of "we usually don't merge style-ish refactors".

Having said that, I did wonder while reading the diff what's the point of providing settings and modify_settings from the SimpleTestCase class, given that these are just wrapped calls to the helper functions override_settings and modify_settings. Was there ever a conversation about removing/deprecating these?

[ngnpope]

I've reviewed this work and I agree that having settings and modify_settings being static methods is both more correct and more handy. So from that POV I would certainly merge the first commit.

If we're keeping these shortcuts then, yes, this makes sense.

I'm a bit reluctant about commits 2 and 3, considering our rule of "we usually don't merge style-ish refactors".

It's not really style-ish refactoring. It's arguably using the preferred approach, given the methods on the test case class are documented ahead of the general functions...

Having said that, I did wonder while reading the diff what's the point of providing settings and modify_settings from the SimpleTestCase class, given that these are just wrapped calls to the helper functions override_settings and modify_settings. Was there ever a conversation about removing/deprecating these?

No, there wasn't, but I see the appeal in getting rid of them - they're partially flawed without the first commit above, and the most common use is as a decorator on the test method or test case class in cases where the override isn't dynamic. Really they only benefit they provide is not needing the import, but that is rare because the more useful, more common decorator usage requires the import.

SimpleTestCase.modify_settings() was added in 5241763 back in 2013 when @modify_settings(...) was added for consistency with SimpleTestCase.settings().

It seems that SimpleTestCase.settings() was actually the original and was kept for backward compatibility when the decorator form of @override_settings(...) (and the setting_changed signal) was added in a3a53e0 back in 2011.

Here are some naive counts of usage in Django itself:

$ git grep -hc @override_settings | paste -s -d+ - | bc
1224
$ git grep -hc @modify_settings | paste -s -d+ - | bc
83
$ git grep -hc 'self.settings(' | paste -s -d+ - | bc
274
$ git grep -hc 'self.modify_settings(' | paste -s -d+ - | bc
13

So, do we want to make this less TIMTOWTDI and deprecate?

[nessita]

@ngnpope Hi and thank you for your patience.

I've been thinking about this, and while I like the idea of the cleanup, I wonder how invasive it could be for many code bases out there.

Would you be willing to propose this in the forum to get a sense of the overall acceptance/rejection of the cleanup idea?

/remind me to check on this in one month

[github-actions]

@nessita set a reminder for 4/19/2024

[github-actions]

👋 @nessita, check on this

[nessita]

I've continued my thinking about this and I do believe now that we should not remove self.settings nor self.modify_settings.
I'm also convinced landing commit 1 is a good idea, but regarding commit 2 and 3 I'm still undecided. @sarahboyce would you have an opinion with your fresh view?

[ngnpope]

Thanks for the follow up @nessita.

So, I agree that the first commit makes sense as it makes the methods on SimpleTestCase less restrictive in where they can be used. But they're still less useful than the decorators.

Certainly we can ignore the other two commits - my opinion changed. Even if we don't want to go in the opposite direction, however, and deprecate these methods, I think we should perhaps invert the documentation order to promote the decorators first as they can be used more flexibly (in more contexts) than the methods can.

[nessita]

I think we should perhaps invert the documentation order to promote the decorators first as they can be used more flexibly (in more contexts) than the methods can.

I think I agree. The docs could use a minor refactor to present the helpers in a way where the self.... methods are presented as convenience TestCase methods (which is what they are right now).

Wanna give it a try @ngnpope?

/remind me about this in one month

[github-actions]

@nessita set a reminder for 5/22/2024

[sarahboyce]

@sarahboyce would you have an opinion with your fresh view?

Nothing fresh to offer, commit 1 and inverting the documentation makes sense to me 👍