-
-
-
-
-
-
djblets/exthandler/forms.py (Diff revision 1) Col: 1 E101 indentation contains mixed spaces and tabs
-
-
[WIP] Extension handler app: New eggs can be installed on the fly with a remote URL.
Review Request #3795 — Created Feb. 2, 2013 and discarded
Started on the extension handler module for djblets. Christian suggested that the extension browse/install/remove be a new app. These changes bring in a new app to djblets called "exthandler". Currently, you can visit <rb>/admin/extensions/manager to view the changes. For now, you can install an extension on the fly. Point the URL field to an archive containing a RB extension (or any python egg for that matter), provide the package name and it'll be installed in the background. Once done, you can enable the newly installed extension at /admin/extensions. Known issues/limitations: -If you try to install an extension that has previously been installed, it doesn't handle it quite well. Some searching ( http://trac.edgewall.org/ticket/7014 ) shows that it's a known issue. Since the already installed extension is being imported by the code elsewhere, the request fails. I'm yet to fix this. -The archive you input has to have the setup.py file directly within; not in a subdirectory. -If you enter a malformed URL (say a random page which leads to a 404) it isn't handled well. I'm yet to fix this too. Some changes are dependant on reviewboard codebase too -- a new URL is added to the RB url.py that handles all /admin/extensions/manager/* urls to exthandler and the app exthandler itself has to be added to RB's settings.py under INSTALLED_APPS otherwise the templating fails. Also fixed entry_points issue on the main extension module and hence the changes to djblets/extensions/base.py.
Tested with two RB extensions. To test make sure you *don't* have reviewbot and/or rbxmlreview installed and they don't show up at /admin/extensions/ 1) Test with rbxmlreview. Provide "http://andromeda.ayrus.net/rbxmlreview.zip" as the URL and "rbxmlreview" as the package name. A success message is expected. Click on the activate link and rbxmlreview should be added to the extensions list on the interface. 2) Test with Review Bot. Provide "http://andromeda.ayrus.net/extension.zip" as the URL and "review-bot-extension" as the package name. A success message is expected. Click on the activate link and Review Bot should be added to the extensions list on the interface.
| Description | From | Last Updated |
|---|---|---|
|
Col: 1 W191 indentation contains tabs |
 reviewbot
|
|
|
Col: 1 W191 indentation contains tabs |
reviewbot
|
|
|
Col: 1 W191 indentation contains tabs |
reviewbot
|
|
|
Col: 5 E901 IndentationError |
reviewbot
|
|
|
Col: 1 E101 indentation contains mixed spaces and tabs |
reviewbot
|
|
|
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot
|
|
|
We should avoid the tag. It's rarely needed, and implies more about the spacing and style than the HTML should. … |
 chipx86
|
|
|
Docstrings should always be in this format: """One-line summary without wrapping. Description. """ |
chipx86
|
|
|
Should be extension_url. |
chipx86
|
|
|
No need for the blank line between these. You will need to localize these. If you look at other files, … |
chipx86
|
|
|
No blank line after the 1 space indentation in HTML. |
chipx86
|
|
|
No spaces in the {{..}} |
chipx86
|
|
|
We apply indentation to our template tags inside the {% .. %}, not outside. So: {% if user_message %} {% … |
chipx86
|
|
|
Indentation is off. This should be like: urlpatterns = patterns(..., url(...), ) Note also that you should be using url() … |
chipx86
|
|
|
Imports should be in alphabetical order. |
chipx86
|
|
|
Missing a docstring. |
chipx86
|
|
|
These blank lines should be gone. |
chipx86
|
|
|
This one too. |
chipx86
|
|
|
And this. |
chipx86
|
|
|
These too (these statements are all related, so they should be grouped). |
chipx86
|
|
|
Put this inside the except. |
chipx86
|
|
|
No blank line here. |
chipx86
|
|
|
locals() should never be passed in. Instead, you should pass in a RequestContext(request, {...}) as the second parameter and be … |
chipx86
|
|
|
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot
|
|
|
Goes inside the except: |
chipx86
|
|
|
No blank line. |
chipx86
|
|
|
The template should be passed the exception as an error, and should render the text itself, rather than being provided … |
chipx86
|
|
|
Same comment about locals. |
chipx86
|
|
|
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot
|
|
|
Not sure what this is? Probably can go away if we do the above (passing in the error and having … |
chipx86
|
|
|
These blank lines should go away. |
chipx86
|
|
|
Same comment here. It occurs to me that we're rendering the same template over and over. We should ideally do … |
chipx86
|
|
|
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (83 > 79 characters) |
reviewbot
|
|
|
Col: 5 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 15 E271 multiple spaces after keyword |
reviewbot
|
|
|
Col: 13 E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
Col: 9 E123 closing bracket does not match indentation of opening bracket's line |
reviewbot
|
|
|
Col: 5 E128 continuation line under-indented for visual indent |
reviewbot
|
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: djblets/extensions/base.py djblets/exthandler/urls.py djblets/settings.py djblets/exthandler/forms.py djblets/exthandler/views.py Ignored Files: djblets/extensions/templates/extensions/extension_list.html .gitignore djblets/exthandler/templates/exthandler/extension_manager.html -
-
AY
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
-
This is a great first stab. There's going to be a lot of comments in here though about fixing style issues and being more in line with the rest of our codebase in ways. I'd suggest a change in the organization of the code. When I was discussing adding a new module, my thoughts were that we'd have one that's specific to browsing for remote extensions (communicating with a remote Extension Store). I'd suggest naming this module "extensionbrowser". I consider things like installing an extension to be more base-level extension functionality, which should live in ExtensionManager. The form you have is probably best suited for extensionbrowser (since eventually we'd have a more full UI on top of it), and it would call out to, say, ExtensionBrowser.install_extension_url.
-
djblets/extensions/templates/extensions/extension_list.html (Diff revision 2) We should avoid the <br /> tag. It's rarely needed, and implies more about the spacing and style than the HTML should. Instead, there should be something like a <ul class="actions">, with this being one of them. Instead of hard-coding a URL, we want to use Django's URL resolution stuff. So, the URL registration for the Add Extension page would have a name (say, "add-extension"), and this would reference it with: {% url "add-extension" %} -
djblets/exthandler/forms.py (Diff revision 2) Docstrings should always be in this format: """One-line summary without wrapping. Description. """
-
-
djblets/exthandler/forms.py (Diff revision 2) No need for the blank line between these. You will need to localize these. If you look at other files, you should see where "_" is imported and used like _("Extension URL"). These mark the strings for translation and handle looking up from the appropriate language bundle. -
djblets/exthandler/templates/exthandler/extension_manager.html (Diff revision 2) No blank line after the <div> 1 space indentation in HTML.
-
djblets/exthandler/templates/exthandler/extension_manager.html (Diff revision 2) No spaces in the {{..}} -
djblets/exthandler/templates/exthandler/extension_manager.html (Diff revision 2) We apply indentation to our template tags inside the {% .. %}, not outside. So: {% if user_message %} {% if user_message == 1 %} etc. These should also be indented by one more, since they're inside the {% block %}. In this case, though, we should do what I said in another comment and check if an error is passed, and then operate based on whether that has a value. -
djblets/exthandler/urls.py (Diff revision 2) Indentation is off. This should be like: urlpatterns = patterns(..., url(...), ) Note also that you should be using url() instead of just a tuple. This will allow you to give it a name (url(..., name="add-extension")) for referencing in the template (see my other comment on this). -
-
-
-
-
-
djblets/exthandler/views.py (Diff revision 2) These too (these statements are all related, so they should be grouped).
-
-
-
djblets/exthandler/views.py (Diff revision 2) locals() should never be passed in. Instead, you should pass in a RequestContext(request, {...}) as the second parameter and be explicit about the variables you provide. The context_instance= shouldn't be there. Just this format: return render_to_response(template_name, RequestContext(request, { 'blah': 'blah', ... }) -
-
-
djblets/exthandler/views.py (Diff revision 2) The template should be passed the exception as an error, and should render the text itself, rather than being provided the text.
-
-
djblets/exthandler/views.py (Diff revision 2) Not sure what this is? Probably can go away if we do the above (passing in the error and having the template conditionally check it and display its own thing).
-
-
djblets/exthandler/views.py (Diff revision 2) Same comment here. It occurs to me that we're rendering the same template over and over. We should ideally do it only once at the end, and prepare the contents for it above.
AY
Change Summary:
-Refactored the changed to a module named "extensionbrowser" instead. --For now the installation specific code lives in the same module, I'll shift it to the built in exthandler eventually. --consequently the URL scheme is now "<rb>/admin/extensions/browser". -Fixed the issues from the review. --Didn't fix two open issues, pending discussion.
Diff: |
Revision 3 (+118 -7) |
|---|
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: djblets/extensions/base.py djblets/exthandler/urls.py djblets/settings.py djblets/exthandler/forms.py djblets/exthandler/views.py Ignored Files: djblets/extensions/templates/extensions/extension_list.html .gitignore djblets/exthandler/templates/exthandler/extension_manager.html -
-
AY
Change Summary:
-Refactored the changed to a module named "extensionbrowser" instead. --For now the installation specific code lives in the same module, I'll shift it to the built in exthandler eventually. --consequently the URL scheme is now "<rb>/admin/extensions/browser". -Fixed the issues from the review. --Didn't fix two open issues, pending discussion.
Diff: |
Revision 4 (+117 -7) |
|---|
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: djblets/extensionbrowser/urls.py djblets/extensionbrowser/forms.py djblets/extensions/base.py djblets/settings.py djblets/extensionbrowser/views.py Ignored Files: djblets/extensions/templates/extensions/extension_list.html .gitignore djblets/extensionbrowser/templates/extensionbrowser/extension_manager.html -
djblets/extensionbrowser/urls.py (Diff revision 4) Col: 5 E128 continuation line under-indented for visual indent
-
-
djblets/extensionbrowser/views.py (Diff revision 4) Col: 13 E126 continuation line over-indented for hanging indent
-
djblets/extensionbrowser/views.py (Diff revision 4) Col: 9 E123 closing bracket does not match indentation of opening bracket's line
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: djblets/extensionbrowser/urls.py djblets/extensionbrowser/forms.py djblets/extensions/base.py djblets/settings.py djblets/extensionbrowser/views.py Ignored Files: djblets/extensions/templates/extensions/extension_list.html .gitignore djblets/extensionbrowser/templates/extensionbrowser/extension_manager.html -
djblets/extensionbrowser/urls.py (Diff revision 5) Col: 5 E128 continuation line under-indented for visual indent
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: djblets/extensions/base.py djblets/exthandler/urls.py djblets/settings.py djblets/exthandler/forms.py djblets/exthandler/views.py Ignored Files: djblets/extensions/templates/extensions/extension_list.html .gitignore djblets/exthandler/templates/exthandler/extension_manager.html