New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
blueyed opened this Issue Nov 21, 2016 · 11 comments

Comments

Projects
None yet
4 participants
@blueyed
Copy link
Contributor

blueyed commented Nov 21, 2016

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

@blueyed blueyed added this to the 3.1 milestone Nov 21, 2016

@peterlauri

This comment has been minimized.

Copy link
Contributor

peterlauri commented Nov 21, 2016

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

@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Nov 21, 2016

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

@blueyed blueyed changed the title Prefix new fixtures with "django_" Prefix new fixtures with "django_"/"dj_" Nov 21, 2016

@peterlauri

This comment has been minimized.

Copy link
Contributor

peterlauri commented Nov 21, 2016

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
#432 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ADWiQUHYv0tAWlH8ecYDwzS_1rHTyickks5rAhJ5gaJpZM4K4gdu
.

@blueyed blueyed modified the milestones: 3.2, 3.1 Nov 21, 2016

@pelme

This comment has been minimized.

Copy link
Member

pelme commented Nov 23, 2016

👍

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

This comment has been minimized.

Copy link
Contributor

peterlauri commented Nov 23, 2016

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

This comment has been minimized.

Copy link
Contributor

peterlauri commented Nov 23, 2016

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

This comment has been minimized.

Copy link
Contributor

tony commented Jan 18, 2018

I have two opinions:

  • To use dj_ as a prefix instead of django_. The text real estate in tests is valuable.

  • Old fixtures should be renamed to use dj_ as a prefix, by the way of a deprecation warning for a few versions

    This can be done via moving all fixtures to the prefix in the next version, and have the old fixtures function names wrap the new, except add a django warning for the user to switch to the new naming.

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

This comment has been minimized.

Copy link
Contributor

blueyed commented Jan 18, 2018

The text real estate in tests is valuable.

Good point.

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

@tony

This comment has been minimized.

Copy link
Contributor

tony commented Jan 18, 2018

@blueyed I'm fine with that.

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

@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Jan 18, 2018

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

This comment has been minimized.

Copy link
Contributor

tony commented Jan 18, 2018

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 #432 (comment) 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 blueyed modified the milestones: 3.2, 3.3 Apr 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment