Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#19333 closed Bug (fixed)

Collect static copies a .py file

Reported by: Camilo Nova Owned by: nobody
Component: contrib.admin Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When you run a collectstatic the file compress.py gets copied to the static directory, and that doesn't seem right.

I have created a pull request because this was generating PEP8 warnings for a project, but looking at the comments i agree this file shouldn't be copied.

https://github.com/django/django/pull/474

Maybe some can tweak the collectstatic command so it ignores .py files.

Change History (16)

comment:1 Changed 4 years ago by Aymeric Augustin

Component: contrib.staticfilescontrib.admin
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted
Type: Cleanup/optimizationBug

Yes, this file isn't designed to be served, it should be moved outside of the admin's static directory.

comment:2 Changed 4 years ago by Jannis Leidel

Or we could add .py to the list of ignore file extensions.

comment:3 Changed 4 years ago by Camilo Nova

Ignoring the .py file extensions sounds a better idea overall

comment:4 Changed 4 years ago by Aymeric Augustin

My personal website offers some Python scripts for download. I'm currently storing them as static files. If .py files were ignored by collectstatic they wouldn't be served anymore.

I may be abusing the static files framework :) but it's possible that other people are doing the same thing. For this reason, "ignore .py files" wasn't my first choice, but I would understand it.

comment:5 Changed 4 years ago by Russell Keith-Magee

Can anyone explain why the compress.py file is actually in the static directory to start with? If the static directory is the directory of stuff you want to share, It seems to me that we could avoid this whole problem by putting compress.py somewhere else (like a utilities directory -- which is what compress.py is anyway)

comment:6 Changed 4 years ago by Ramiro Morales

Has patch: set

comment:7 Changed 4 years ago by Camilo Nova

Ramiro, looks good and clean. Thanks

comment:8 Changed 4 years ago by Aymeric Augustin

After your changes the script works only if you pass file names in argument or if you run it from django/contrib/admin/static/admin/js.

The here variable should be called admin_js_dir, because that's what it really is :)

I suggest renaming that variable and pointing it to os.path.join(os.path.dirname(os.path.dirname(__file__)), 'static', 'admin', 'js') (to be tested, I just wrote it in this comment as an example).

Otherwise, that's the correct fix IMO.

Last edited 4 years ago by Aymeric Augustin (previous) (diff)

comment:9 in reply to:  8 Changed 4 years ago by Ramiro Morales

Replying to aaugustin:

After your changes the script works only if you pass file names in argument or if you run it from django/contrib/admin/static/admin/js.

According to my understanding of the code it's also the current behavior. No changes here.

The here variable should be called admin_js_dir, because that's what it really is :)

If I'm right above, the meaning of this var hasn't changed either. It is the CWD from which the script is being executed (not the dir where the script is located).

I suggest renaming that variable and pointing it to os.path.join(os.path.dirname(os.path.dirname(__file__)), 'static', 'admin', 'js') (to be tested, I just wrote it in this comment as an example).

Otherwise, that's the correct fix IMO.

The only change this patch introduces is that to ejecute compress.py one needs to specify its full path (e.g. python ../../../bin/compress.py). Remember the intended potential uses of this script are core devs and people contributing with the admin app by submitting JS patches.

Last edited 4 years ago by Ramiro Morales (previous) (diff)

comment:10 Changed 4 years ago by Julien Phalip

Triage Stage: AcceptedReady for checkin

comment:11 Changed 4 years ago by Julien Phalip

Never mind, I got a little confused. The patch already modifies the doc...

On second thought, I agree with Aymeric in comment:8 to not use getcwd(), so that the command can be run from anywhere.

comment:12 Changed 4 years ago by Julien Phalip

Triage Stage: Ready for checkinAccepted

comment:13 Changed 4 years ago by Ramiro Morales

Triage Stage: AcceptedReady for checkin

comment:14 Changed 4 years ago by Julien Phalip <jphalip@…>

Resolution: fixed
Status: newclosed

In c9c40bc6bc64e67365338751e4967d86d0882abf:

Fixed #19333 -- Moved compress.py outside of the admin static folder. Thanks to camilonova, Russell Keith-Magee, Aymeric Augustin and Ramiro Morales for the feedback.

comment:15 Changed 4 years ago by Julien Phalip <jphalip@…>

In be5369fd24adfc788e59eaf0363d4d137ded6ef8:

[1.5.x] Fixed #19333 -- Moved compress.py outside of the admin static folder. Thanks to camilonova, Russell Keith-Magee, Aymeric Augustin and Ramiro Morales for the feedback.
Backport of c9c40bc6bc64e6

comment:16 Changed 4 years ago by Julien Phalip <jphalip@…>

In be5369fd24adfc788e59eaf0363d4d137ded6ef8:

[1.5.x] Fixed #19333 -- Moved compress.py outside of the admin static folder. Thanks to camilonova, Russell Keith-Magee, Aymeric Augustin and Ramiro Morales for the feedback.
Backport of c9c40bc6bc64e6

Note: See TracTickets for help on using tickets.
Back to Top