Changable compressor class #354
Conversation
PetrDlouhy
force-pushed
the
changable_compressor_class
branch
2 times, most recently
from
c114d60
to
128b704
Compare
|
@adamchainz could this be merged once rebase? I think this feature would be great to give users flexibility. |
There was a problem hiding this comment.
I haven't thought too hard about whether this is a good solution. It's a large API area to expose to users.
The PR is also far from ready: there are no tests, the documentation needs expanding significantly to describe how to subclass Compressor and what methods can be customized, there are no non-django docs, and there's no changelog entry.
I would also like to see some evidence of experimentation with alternative approaches, even just monkey-patching the current version of Whitenoise to see how easy it is to skip files that way.
whitenoise/compress.py
Outdated
| try: | ||
| compressor_class = import_string( | ||
| getattr( | ||
| settings, "WHITENOISE_COMPRESSOR_CLASS", "whitenoise.compress.Compressor" | ||
| ), | ||
| ) | ||
| except ImproperlyConfigured: | ||
| compressor_class = Compressor |
There was a problem hiding this comment.
reading settings at import time is not generally safe in Django, since they can still be overriden, in e.g. tests or by later configuration settings loading. Only read the setting at runtime
PetrDlouhy
force-pushed
the
changable_compressor_class
branch
3 times, most recently
from
9335692
to
0fec8c0
Compare
|
@adamchainz I have rebased to current Have you made some decision whether you want this in the project or not? I would like to work on the tests and documentation, but I don't want to waste my time if it doesn't eventually get merged.
I am not sure how to deal with this. Of course it would be possible with monkey-patching, but it would be possible, but it would be very ugly. |
src/whitenoise/compress.py
Outdated
| "whitenoise.compress.Compressor", | ||
| ), | ||
| ) | ||
| except ImproperlyConfigured: |
There was a problem hiding this comment.
Is this still needed now that the settings import is inside a function?
I worry that it would hide legitimate problems without any notice to the dev.
There was a problem hiding this comment.
@merwok You are right. I fixed it.
PetrDlouhy
force-pushed
the
changable_compressor_class
branch
from
66fece3
to
042dd97
Compare
for more information, see https://pre-commit.ci
Fixes #279.