Remove automatic setup feature #851

jdufresne · 2016-07-16T15:36:19Z

Fixes #838

aaugustin · 2016-07-21T08:48:08Z

I'm going to make a release today to fix an incompatibility with a recent release of sqlparse.

I'm not ready to make this change in a hurry, but it's still on the table.

Perhaps you could bring the idea up on django-users?

codecov-io · 2016-08-19T14:38:10Z

Current coverage is 76.86% (diff: 86.66%)

Merging #851 into master will increase coverage by 1.33%

@@             master       #851   diff @@
==========================================
  Files            30         30          
  Lines          1659       1634    -25   
  Methods           0          0          
  Messages          0          0          
  Branches        246        244     -2   
==========================================
+ Hits           1253       1256     +3   
+ Misses          327        302    -25   
+ Partials         79         76     -3

Powered by Codecov. Last update d27f4ff...fe74d9b

timgraham · 2016-08-19T21:06:28Z

debug_toolbar/settings.py

-
-# The following functions can monkey-patch settings automatically. Several
-# imports are placed inside functions to make it safe to import this module.
+# Several imports are placed inside functions to make it safe to import this


It looks like only 1 function is left. Do you understand the comment? It's not clear to me who is doing the importing of this module.

jdufresne · 2016-08-20T15:13:02Z

@matthiask I think some of the doc changes from this commit were mistakenly included in 388e177.

matthiask · 2016-08-20T15:29:54Z

@jdufresne Oops. Thank you for the report!

matthiask · 2016-08-20T15:37:29Z

Request for comments: https://groups.google.com/forum/#!topic/django-users/no_yOMvFZ0M (no replies yet)

Thanks for the feedback @timgraham!

jdufresne · 2016-08-20T16:25:35Z

Thanks for the feedback. I believe I have addressed the issues raised. All additional feedback is welcome. I have tested the changes using the project's unit tests and against my own Django applications.

It looks like only 1 function is left. Do you understand the comment? It's not clear to me who is doing the importing of this module.

In the latest version I have moved check_middleware() to apps.py. This is the only file that calls this function. Moving it allowed me remove the imports inside the function; they are now at the top level. I removed the comment entirely as I feel it is no longer relevant.

It would be nice to explain why it's been removed.

I adapted the warnings from the old installation docs. Those previous warnings are now listed as reasons for removing the automatic setup feature in the changes docs. Let me know what you think.

matthiask · 2016-08-20T16:39:34Z

debug_toolbar/apps.py

+    debug_toolbar_index = None
+
+    # Determine the indexes which gzip and/or the toolbar are installed at
+    for i, middleware in enumerate(settings.MIDDLEWARE_CLASSES):


MIDDLEWARE should also be checked, not only MIDDLEWARE_CLASSES. (IMO the check could also be replaced by a big warning in the installation docs, but not sure whether that would be alright with everyone.)

IMO the check could also be replaced by a big warning in the installation docs, but not sure whether that would be alright with everyone.

This already exists to some extent. From the installation docs Middleware section:

The order of MIDDLEWARE_CLASSES is important. You should include the Debug
Toolbar middleware as early as possible in the list. However, it must come
after any other middleware that encodes the response's content, such as
:class:~django.middleware.gzip.GZipMiddleware.

Is this sufficient? Or would you like me to expand on this blurb? Or should I continue with modifying the runtime check?

matthiask · 2016-08-20T18:17:40Z

I feel a bit bad making those judgement calls seeing as I'm not contributing to DDT for a long time (at all). But let's continue this :-)

I very much prefer checking MIDDLEWARE first, since I don't think that removing the check removes much complexity from the codebase, and it's one stumbling block less when adding DDT to a project.

jdufresne · 2016-08-20T18:34:02Z

Updated the runtime check to handle MIDDLEWARE
Updated the runtime check to warn on the absence of DebugToolbarMiddleware (as it is no longer added automatically)
Updated the warning wording based on removal of automatic setup
Emphasized the GZipMiddleware documentation as a "warning"

matthiask · 2016-08-20T18:49:23Z

Thank you! Now everything looks good to me. I'm not worried about the unsuccessful codecov/patch check in this case.

Let's wait a working day or two for feedback on the mailing list and here on the pull request. I'll merge in case nobody brings up some good points why this shouldn't be done.

Thanks again!

jdufresne · 2016-08-20T19:28:10Z

I only just now realized that DDT includes an example project (for testing?). Would you like this updated as well to use the explicit setup? Should be easy to update.

aaugustin · 2016-08-20T19:34:18Z

Yes, the example project is for manual testing and should be updated to reflect whatever the documentation recommends.

jdufresne · 2016-08-20T19:53:09Z

Thanks. The example project has been updated to current Django and DDT configuration recommendations.

timgraham · 2016-08-21T00:43:54Z

debug_toolbar/apps.py

+        check_middleware()
+
+
+def check_middleware():


If not too complicated, that might be a candidate for using the system check framework.

The middleware check was moved to apps.py and changed to use the system checks framework. It was updated to check MIDDLEWARE in addition to MIDDLEWARE_CLASSES. It now checks that DebugToolbarMiddleware occurs in one of the settings. Updated installation documentation to reflect new requirements. The bundled example project was updated to current Django and Debug Toolbar configuration recommendations. Fixes #838

jdufresne · 2016-08-21T05:46:10Z

If not too complicated, that might be a candidate for using the system check framework.

Great idea! I have updated the change to use system checks. I also added a test for these systems checks so codecov is happy again.

I made other suggested changes as well. Thanks!

timgraham reviewed Aug 19, 2016
View reviewed changes

matthiask reviewed Aug 20, 2016
View reviewed changes

timgraham reviewed Aug 21, 2016
View reviewed changes

matthiask merged commit fe74d9b into django-commons:master Aug 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove automatic setup feature #851

Remove automatic setup feature #851

jdufresne commented Jul 16, 2016

aaugustin commented Jul 21, 2016

codecov-io commented Aug 19, 2016 •

edited

Loading

timgraham Aug 19, 2016

jdufresne commented Aug 20, 2016 •

edited

Loading

matthiask commented Aug 20, 2016

matthiask commented Aug 20, 2016

jdufresne commented Aug 20, 2016

matthiask Aug 20, 2016 •

edited

Loading

jdufresne Aug 20, 2016

matthiask commented Aug 20, 2016

jdufresne commented Aug 20, 2016

matthiask commented Aug 20, 2016

jdufresne commented Aug 20, 2016

aaugustin commented Aug 20, 2016

jdufresne commented Aug 20, 2016

timgraham Aug 21, 2016

jdufresne commented Aug 21, 2016 •

edited

Loading

Remove automatic setup feature #851

Remove automatic setup feature #851

Conversation

jdufresne commented Jul 16, 2016

aaugustin commented Jul 21, 2016

codecov-io commented Aug 19, 2016 • edited Loading

Current coverage is 76.86% (diff: 86.66%)

timgraham Aug 19, 2016

Choose a reason for hiding this comment

jdufresne commented Aug 20, 2016 • edited Loading

matthiask commented Aug 20, 2016

matthiask commented Aug 20, 2016

jdufresne commented Aug 20, 2016

matthiask Aug 20, 2016 • edited Loading

Choose a reason for hiding this comment

jdufresne Aug 20, 2016

Choose a reason for hiding this comment

matthiask commented Aug 20, 2016

jdufresne commented Aug 20, 2016

matthiask commented Aug 20, 2016

jdufresne commented Aug 20, 2016

aaugustin commented Aug 20, 2016

jdufresne commented Aug 20, 2016

timgraham Aug 21, 2016

Choose a reason for hiding this comment

jdufresne commented Aug 21, 2016 • edited Loading

codecov-io commented Aug 19, 2016 •

edited

Loading

jdufresne commented Aug 20, 2016 •

edited

Loading

matthiask Aug 20, 2016 •

edited

Loading

jdufresne commented Aug 21, 2016 •

edited

Loading