Fixed #22557 -- ManifestStaticFilesStorage did not cleanup deleted files #2662

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@syphar
Contributor

syphar commented May 16, 2014

When using ManifestStaticFilesStorage, deleted static files would be
correctly cleaned up by "collectstatic --clear", but the manifest file
would still contain the stale entries.

Thanks to tedtieken for the report

Fixed #22557 -- ManifestStaticFilesStorage did not cleanup deleted files
When using ManifestStaticFilesStorage, deleted static files would be
correctly cleaned up by "collectstatic --clear", but the manifest file
would still contain the stale entries.

Thanks to tedtieken for the report
@apollo13

This comment has been minimized.

Show comment Hide comment
@apollo13

apollo13 May 16, 2014

Owner

@jezdez Looks good to me, can you give it a glimpse and merge it? the caching backend is ignored on purpose, cause clear flushes the whole cache and the items usually expire anyways…

Owner

apollo13 commented May 16, 2014

@jezdez Looks good to me, can you give it a glimpse and merge it? the caching backend is ignored on purpose, cause clear flushes the whole cache and the items usually expire anyways…

@tedtieken

This comment has been minimized.

Show comment Hide comment
@tedtieken

tedtieken May 16, 2014

Can we still break post_processed into two parts, post_processing and saving the manifest?

Can we still break post_processed into two parts, post_processing and saving the manifest?

@apollo13

This comment has been minimized.

Show comment Hide comment
@apollo13

apollo13 May 16, 2014

Owner

@tedtieken why? none of this is really public API, so I rather leave it as is and move out if actually needed.

Owner

apollo13 commented May 16, 2014

@tedtieken why? none of this is really public API, so I rather leave it as is and move out if actually needed.

@tedtieken

This comment has been minimized.

Show comment Hide comment
@tedtieken

tedtieken May 16, 2014

Because it makes subclassing possible with less headaches.

By setting the hashed_files to {} and saving in the same method, if I subclass ManifestFilesMixin and do anything interesting, I have to rewrite the entire post_process function instead of just the post_processing.

I don't think this approach to the problem is a very good one, because it means you can't do anything in post_process in a sub class:

Consider the following scenario:

class MyStorage(ManifestFilesMixin, etc.):
    def post_process(self, *args, **kwargs):
        super(MyStorage, self).post_process(*args, **kwargs)
        ....
        can't do custom stuff here because the manifest has already been written to disk
        .....


class MyStorage(ManifestFilesMixin, etc.):
    def post_process(self, *args, **kwargs):
        ....
        can't do custom stuff here because hashed_files dict will be reset 
        in ManifestFilesMixin.post_process
        .....
        super(MyStorage, self).post_process(*args, **kwargs)

So, if I need to do post_processing work, I have to skip over manifestfiles mixin

class MyStorage(ManifestFilesMixin, etc.):
    def post_process(self, *args, **kwargs):
        super(HashedFilesMixin, self).post_process(*args, **kwargs)
        .....
        Now I can do custom stuff
        ....
        But if I do, I have to re-implement the missing save_manifest logic here
        ....

In this scenario, I have to hande the logic of writing the manifest file to disk in my subclass.

Whereas, if ManifestFilesMixin did post_process and save_manifest separately, I could still use the save_manifest method and only have to skip over post_process -- still not the most elegant, but far less of a kludge.

Why would I want to do this? Pipelining assets off of collect static. HashedFilesMixin gets us fingerprinting, I've written a library that extends that to also minify and gzip my assets. This isn't a theoretical issue -- If the code goes into the release like this, I will have to make changes to my library that feel wrong. Subclasses shouldn't be handling the logic to write the file to disk when they aren't handling the reading logic. A subclass re-implementing verbatim parts of the parent class is a code smell and one I'd like to try to avoid.

Because it makes subclassing possible with less headaches.

By setting the hashed_files to {} and saving in the same method, if I subclass ManifestFilesMixin and do anything interesting, I have to rewrite the entire post_process function instead of just the post_processing.

I don't think this approach to the problem is a very good one, because it means you can't do anything in post_process in a sub class:

Consider the following scenario:

class MyStorage(ManifestFilesMixin, etc.):
    def post_process(self, *args, **kwargs):
        super(MyStorage, self).post_process(*args, **kwargs)
        ....
        can't do custom stuff here because the manifest has already been written to disk
        .....


class MyStorage(ManifestFilesMixin, etc.):
    def post_process(self, *args, **kwargs):
        ....
        can't do custom stuff here because hashed_files dict will be reset 
        in ManifestFilesMixin.post_process
        .....
        super(MyStorage, self).post_process(*args, **kwargs)

So, if I need to do post_processing work, I have to skip over manifestfiles mixin

class MyStorage(ManifestFilesMixin, etc.):
    def post_process(self, *args, **kwargs):
        super(HashedFilesMixin, self).post_process(*args, **kwargs)
        .....
        Now I can do custom stuff
        ....
        But if I do, I have to re-implement the missing save_manifest logic here
        ....

In this scenario, I have to hande the logic of writing the manifest file to disk in my subclass.

Whereas, if ManifestFilesMixin did post_process and save_manifest separately, I could still use the save_manifest method and only have to skip over post_process -- still not the most elegant, but far less of a kludge.

Why would I want to do this? Pipelining assets off of collect static. HashedFilesMixin gets us fingerprinting, I've written a library that extends that to also minify and gzip my assets. This isn't a theoretical issue -- If the code goes into the release like this, I will have to make changes to my library that feel wrong. Subclasses shouldn't be handling the logic to write the file to disk when they aren't handling the reading logic. A subclass re-implementing verbatim parts of the parent class is a code smell and one I'd like to try to avoid.

@timgraham

This comment has been minimized.

Show comment Hide comment
@timgraham

timgraham Jun 20, 2014

Owner

merged in 3bec388.

Owner

timgraham commented Jun 20, 2014

merged in 3bec388.

@timgraham timgraham closed this Jun 20, 2014

@syphar syphar deleted the syphar:ticket22557 branch Apr 3, 2016

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