search

pytest-dev / pytest-django

A Django plugin for pytest.
https://pytest-django.readthedocs.io/
Other
1.37k stars 344 forks source link

Prefix new fixtures with "django_"/"dj_" #432

Open blueyed opened 7 years ago

blueyed commented 7 years ago

IIRC we agreed to make fixture names more explicit / namespaced, e.g. django_mailoutbox. This should be done for new fixtures at least.

peterlauri commented 7 years ago

Good idea... Alternative dj_mailoutbox, just a shorter version... This is something I have been thinking about when using some of the fixtures...

blueyed commented 7 years ago

Yes, a shorter prefix makes sense. So 👍 for dj_ (which would mean to rename/move some of the existing django_ ones, too).

peterlauri commented 7 years ago

Maybe rename all public fixtures, and keep the old ones as aliases for a few releases?

On Mon, Nov 21, 2016 at 10:51 PM Daniel Hahler notifications@github.com wrote:

Yes, a shorter prefix makes sense. So 👍 for dj_.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pytest-dev/pytest-django/issues/432#issuecomment-262078951, or mute the thread https://github.com/notifications/unsubscribe-auth/ADWiQUHYv0tAWlH8ecYDwzS_1rHTyickks5rAhJ5gaJpZM4K4gdu .

pelme commented 7 years ago

👍

We (@blueyed and me) discussed this during the pytest sprint. I forgot about it when reviewing the mailoutbox fixture. :/

Keeping aliases for quite some time and emit pytest warnings from them would be a good way to proceed. :)

peterlauri commented 7 years ago

But related to dj_ vs django_: as you already have quite many fixtures named "correctly" with django_, maybe make sense to keep it? As well as markers...

peterlauri commented 7 years ago 
django_admin_client
django_admin_user
django_client
django_db
django_live_server
django_rf
django_settings
django_transactional_db
django_mailoutbox
...

vs

dj_admin_client
dj_admin_user
dj_client
dj_db
dj_live_server
dj_rf
dj_settings
dj_transactional_db
dj_mailoutbox
tony commented 6 years ago

I have two opinions:

That way, in the next few versions, the old fixtures will be phased out completely, and everything will be consistent for users, and also easier to maintain on pytest-django side.

blueyed commented 6 years ago

The text real estate in tests is valuable.

Good point.

Let's go with dj_ then for new ones?!

tony commented 6 years ago

@blueyed I'm fine with that.

Does this mean to switch the new fixtures in #568 to use dj_?

blueyed commented 6 years ago

Yes, they should be changed.

However, I am not really confident: it means to change / migrate all existing fixtures - saving 4 characters per use of them only. The longer django_ prefix has the benefit of matching the plugin's name (after pytest-). So I would rather hear other opinions / feedback first.

tony commented 6 years ago

However, I am not really confident: it means to change / migrate all existing fixtures - saving 4 characters per use of them only. So I would rather hear other opinions / feedback first.

Okay, so I'll hold off updating #568 to dj_ for the moment. Or that could be merged in as-is if it's okay. Because if we begin migrating old fixtures to dj_, it'd end up being updated anyway. I favor an approach like mentioned in https://github.com/pytest-dev/pytest-django/issues/432#issuecomment-358665256 where a deprecation warning is temporarily given. The new fixtures in #568 wouldn't need a deprecation warning if merged as-is now unless there was a release published between now and transitioning the fixtures to (whichever) new prefix.

The longer django_ prefix has the benefit of matching the plugin's name (after pytest-).

That is an inconsistency and it's noted. I'm not a big fan of dj as a naming convention. But as a pytest prefix for fixtures, the compromise makes sense to me.

The reason I recommend dj_ is specifically due to the code and logic that gets scrunched into tests. Also, since pytest fixtures are in function arguments, the space their is also limited, in many cases, they end up becoming multiple lines very easily. The 4 characters are enough - but I only think it works if we were to tag a release where we did it all at once.

So that's my rationale/vote/opinion on it.

blueyed commented 5 years ago

Ref: https://github.com/pytest-dev/pytest/issues/4900.

I had a quick go on adding a "django_" prefix for all existing fixtures, while still providing the old name. Actually emitting a DeprecationWarning could be done later.

1oglop1 commented 3 years ago

@blueyed Was there any progress on this? I just accidentally overridden client that comes from pytest-django with a model factory created by pytest-factoryboy It took me some time to realize what is going on.