Avoid silently changing settings; raise ImproperlyConfigured instead #520
Conversation
If a setting is considered invalid, raise an exception instead of silently changing it to some other value. Avoids confusion and forces the library user to be explicit. For example, setting S3 location to a value with a leading slash previously stripped it. Instead, now instruct the user to use a valid value without the leading slash.
Codecov Report
@@ Coverage Diff @@
## master #520 +/- ##
=========================================
+ Coverage 76.48% 76.79% +0.3%
=========================================
Files 11 11
Lines 1595 1599 +4
=========================================
+ Hits 1220 1228 +8
+ Misses 375 371 -4
Continue to review full report at Codecov.
|
Fix missing import needed in #520
Fix missing import needed in jschneier#520
|
Sorry I didn't comment on this one. What do you guys think of a system check? An issue at runtime to me should be the last resort. |
|
That would be great. Unfortunately, I believe a system check would require an entry in |
|
This is in the README:
Once that is done add storages to your INSTALLED_APPS and set DEFAULT_FILE_STORAGE to the backend of your choice.
Am I missing something? This is admittedly a piece of Django with which I am not that familiar.
… On Aug 11, 2018, at 9:30 PM, Jon Dufresne ***@***.***> wrote:
That would be great. Unfortunately, I believe a system check would require an entry in INSTALLED_APPS which doesn't exist for django-storages.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#520 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACJB2IU3uQD5mkPAuScx5u-l157YoTMKks5uP4UlgaJpZM4VB7fR>.
|
|
Interesting. Thanks for pointing that out. django-storages doesn't actually have a need for using |
|
I like system checks generally, maybe it is something to consider. I will merge your pull request for now.
… On Aug 11, 2018, at 10:31 PM, Jon Dufresne ***@***.***> wrote:
Interesting. Thanks for pointing that out.
django-storages doesn't actually have a need for using INSTALLED_APPS. We can simplify the installation by removing it. I have been successfully using django-storages without INSTALLED_APPS on projects for years. Using an app would be required for things like models, migrations, templates, and templatetags. The configuration used, DEFAULT_FILE_STORAGE, can be any valid Python path even if it isn't a configured app. With that, I think it would be better to have simpler install instructions. I created PR #547 <#547> to do so.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#520 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACJB2OvmlBb81XVz5hZykJUlRhMrNfKNks5uP5OPgaJpZM4VB7fR>.
|
If a setting is considered invalid, raise an exception instead of silently changing it to some other value. Avoids confusion and forces the library user to be explicit.
For example, setting S3 location to a value with a leading slash previously stripped it. Instead, now instruct the user to use a valid value without the leading slash.