Add support for middleware to extensions

Review Request #4530 — Created Sept. 5, 2013 and submitted

Information

Djblets
master

Reviewers

Add support for middleware in extensions.

All unit tests pass.

Description From Last Updated

Hm... so it sounds like we'd be generating this list pretty much every time the MiddlewareRunner was hit... Could we …

mike_conleymike_conley

Maybe "_recalculate_middleware?"

chipx86chipx86

What is "this" and "dependencies" in theis context? Not sure this paragraph is describing the function.

chipx86chipx86

Blank line between statements and for/if/etc. statements that form blocks.

chipx86chipx86

Should be _get_extension_middleware.

chipx86chipx86

I'd recommend specifying that this is a utility function for _set_middleware, and describe how it's used/what it does, since it …

chipx86chipx86

Blank around each of htese.

chipx86chipx86

Blank before this.

chipx86chipx86

And before this.

chipx86chipx86

And before this.

chipx86chipx86

I don't see where this is used. The class should have a docstring going into detail on what this is …

chipx86chipx86

Blank line before.

chipx86chipx86

And here.

chipx86chipx86

And here.

chipx86chipx86

And here.

chipx86chipx86

And here.

chipx86chipx86

And here.

chipx86chipx86

And here.

chipx86chipx86

And here.

chipx86chipx86

It might be worth documenting here that this class needs to be added to the settings.py MIDDLEWARE_CLASSES for the extension …

SM smacleod

Mark should go before Micah.

mike_conleymike_conley
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    djblets/extensions/base.py
    djblets/extensions/middleware.py
    Ignored Files:
    AUTHORS

  2. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    djblets/extensions/base.py
    djblets/extensions/middleware.py
    Ignored Files:
    AUTHORS

  2. 
      
mike_conley
  1. 
      
    1. Yeah, that's a good idea. I only fully understood how extensions were loaded part-way through writing this patch, so that never occurred to me. Will fix.

  2. djblets/extensions/middleware.py (Diff revision 1)
     
     
     
     
     
     
     
     

    Hm... so it sounds like we'd be generating this list pretty much every time the MiddlewareRunner was hit...

    Could we not have the ExtensionManager maintain this list of middleware, and add / remove to it when extensions are enabled and disabled? This way, we wouldn't need to generate it each time.

    Or am I missing a case?

  3. 
      
MC
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    djblets/extensions/base.py
    djblets/extensions/middleware.py
    Ignored Files:
    AUTHORS

  2. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    djblets/extensions/base.py
    djblets/extensions/middleware.py
    Ignored Files:
    AUTHORS

  2. 
      
MC
chipx86
  1. 
      
  2. djblets/extensions/base.py (Diff revision 2)
     
     

    Maybe "_recalculate_middleware?"

  3. djblets/extensions/base.py (Diff revision 2)
     
     
     

    What is "this" and "dependencies" in theis context? Not sure this paragraph is describing the function.

    1. I was trying to explain that, rather than attempting to insert or delete exactly the middleware (and dependencies) of the enabled/disabled extension, we just throw away the whole list and recalculate it for all extensions. It's probably an unnecessary justification, though, especially after renaming the function as you suggested, so I'll just delete it.

  4. djblets/extensions/base.py (Diff revision 2)
     
     

    Blank line between statements and for/if/etc. statements that form blocks.

  5. djblets/extensions/base.py (Diff revision 2)
     
     

    Should be _get_extension_middleware.

  6. djblets/extensions/base.py (Diff revision 2)
     
     
     

    I'd recommend specifying that this is a utility function for _set_middleware, and describe how it's used/what it does, since it has to operate within _set_middleware.

  7. djblets/extensions/base.py (Diff revision 2)
     
     
     

    Blank around each of htese.

    1. I assume you mean blank lines around this whole block, not around each line.

  8. djblets/extensions/base.py (Diff revision 2)
     
     

    Blank before this.

  9. djblets/extensions/base.py (Diff revision 2)
     
     

    And before this.

  10. djblets/extensions/base.py (Diff revision 2)
     
     

    And before this.

  11. djblets/extensions/middleware.py (Diff revision 2)
     
     
     
     

    I don't see where this is used.

    The class should have a docstring going into detail on what this is for.

    Same with all the functions within.

    1. Heh this is the whole point of the patch. :) However I really should have added some comments, which I will do now.

  12. djblets/extensions/middleware.py (Diff revision 2)
     
     

    Blank line before.

  13. djblets/extensions/middleware.py (Diff revision 2)
     
     

    And here.

  14. djblets/extensions/middleware.py (Diff revision 2)
     
     

    And here.

  15. djblets/extensions/middleware.py (Diff revision 2)
     
     

    And here.

  16. djblets/extensions/middleware.py (Diff revision 2)
     
     

    And here.

  17. djblets/extensions/middleware.py (Diff revision 2)
     
     

    And here.

  18. djblets/extensions/middleware.py (Diff revision 2)
     
     

    And here.

  19. djblets/extensions/middleware.py (Diff revision 2)
     
     

    And here.

  20. 
      
MC
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    djblets/extensions/base.py
    djblets/extensions/middleware.py
    Ignored Files:
    AUTHORS

  2. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    djblets/extensions/base.py
    djblets/extensions/middleware.py
    Ignored Files:
    AUTHORS

  2. 
      
SM
  1. Just a nit about some documentation, otherwise it looks good to me!

  2. djblets/extensions/middleware.py (Diff revision 3)
     
     
     
     
     
     
     
     

    It might be worth documenting here that this class needs to be added to the settings.py
    MIDDLEWARE_CLASSES for the extension middleware to be run.

    1. Hm well, there are other middleware classes in djblets that are loaded by ReviewBoard, and none of them have a comment like that. I was just going to add it to ReviewBoard's settings.py as a separate patch once this one is committed.

    2. Fair enough. You're right, the other middleware isn't documented like that.

    3. It should document, however, whether placement matters, and if so, where it should go.

    4. Hm, so, I don't see an explicit comment in, say, djblets.siteconfig.middleware.SettingsMiddleware, which has a comment above it in reviewboard/settings.py, but I will add one anyway.

  3. 
      
SM
  1. Ship It!

  2. 
      
MC
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    djblets/extensions/base.py
    djblets/extensions/middleware.py
    Ignored Files:
    AUTHORS

  2. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    djblets/extensions/base.py
    djblets/extensions/middleware.py
    Ignored Files:
    AUTHORS

  2. 
      
SM
  1. Ship It!

  2. 
      
mike_conley
  1. Just one minor nit. The rest of this looks fine to me.

  2. AUTHORS (Diff revision 4)
     
     
     

    Mark should go before Micah.

  3. 
      
MC
reviewbot
  1. This is a review from Review Bot.
    Tool: PEP8 Style Checker
    Processed Files:
    djblets/extensions/base.py
    djblets/extensions/middleware.py
    Ignored Files:
    AUTHORS

  2. 
      
reviewbot
  1. This is a review from Review Bot.
    Tool: Pyflakes
    Processed Files:
    djblets/extensions/base.py
    djblets/extensions/middleware.py
    Ignored Files:
    AUTHORS

  2. 
      
mike_conley
  1. Ship It!

  2. 
      
SM
  1. Ship It!

  2. 
      
david
  1. Ship It!

  2. 
      
MC
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.7.x (e28f6d2). Thanks!

Loading...