Extension browser module with search/install feature from a remote store.

Review Request #3906 — Created Feb. 23, 2013 and discarded

Information

Review Board
master

Reviewers

Extension browser module - Supports searching and installing extensions. Extensions are retrieved from a remote extension store and are accessed through the store's API. JSON responses are expected back.

This review request follows: http://www.youtube.com/watch?v=Xvntl7ksjPA&hd=1 (partially).

Change is also dependant on a modification to djblets/extensions/base.py -- method which performs an installation through easy_install given a remote archive containting an egg -- as on /r/3973

 

Description From Last Updated

One thing that strikes me is that when you switch from tab to tab, the size of the modal changes, …

mike_conleymike_conley

Detail - tabs should touch their content.

mike_conleymike_conley

Er, isn't this a change from one of your original easyfix bugs?

mike_conleymike_conley

Imports are always in up to 3 groups: 1) Python-bundled modules 2) Third-party modules (like simplejson) 3) Project's modules (like …

chipx86chipx86

Shouldn't use simplejson. Instead, for now, use django.utils.simplejson. (This is going away, but let's be consistent for now.)

chipx86chipx86

We now have multiple things named Extension. Let's change this one to StoreExtensionInfo.

chipx86chipx86

This is very technical and too specific to the current workflow. This level of code should not know how it …

chipx86chipx86

This is the kind of thing that should be handled by the rendering, not core logic. I'd say just nuke …

chipx86chipx86

The JSON response part is an implementation detail, and not something that needs to be known to consumers of the …

chipx86chipx86

Remove the blank line.

chipx86chipx86

Generally when a list comprehension spans multiple lines, we like to do: self.foo = [ ext.dist.project_name.lower() for ext in .... …

chipx86chipx86

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Use format strings. They're cheaper for Python.

chipx86chipx86

Same. In fact, combine this with the previous line, too.

chipx86chipx86

What happens when/if this fails?

chipx86chipx86

Make sure to change the "Extension" part when renaming the class.

chipx86chipx86

typo: corresponding

mike_conleymike_conley

Since we're using a dictionary, does this mean we can't rely on any certain ordering from the server?

chipx86chipx86

If an extension manager wasn't passed to the constructor, self.installed_extensions will be None, which is not iterable.

mike_conleymike_conley

Given how many arguments we have, and that this will likely grow, can you instead use keyword arguments for this? …

chipx86chipx86

typo: package

mike_conleymike_conley

It may not be a zip file. It could be an egg, or tar.gz, or whatever. Best not to be …

chipx86chipx86

We're repeating the whole HTTP code a couple times. Can this be consolidated into a wrapper function?

chipx86chipx86

One-line summaries shouldn't have both """ on the same line. In this function, though, there's no documentation on what this …

chipx86chipx86

Wouldn't we want to pass the error on to the caller instead of swallowing it silently?

mike_conleymike_conley

Shouldn't have the ":" in the label. That's a rendering detail.

chipx86chipx86

See about using {% static %} for this, since {% admin_media_prefix %} is going away.

chipx86chipx86

Shouldn't use either of these. Instead, use {% compressed_css %} with some name, which you'll register in settings.py in PIPELINE_CSS. …

chipx86chipx86

Should be scripts-post, so that we load this after rendering the page.

chipx86chipx86

Blank line before this.

chipx86chipx86

Always use {% url %} instead of hard-coding URLs.

chipx86chipx86

All of the strings in here should be wrapped in trans blocks

mike_conleymike_conley

Indentation is screwy.

chipx86chipx86

Use {% trans %}

chipx86chipx86

Any IDs we don't need should be removed. Any we do should be properly namespaced.

chipx86chipx86

Use {% trans %} for all these strings. Go through the files and make sure you're always localizing.

chipx86chipx86

Seem to have some tab vs. space problems. Make sure this file has no tabs, and no trailing whitespace.

chipx86chipx86

Classes always use -, not _

chipx86chipx86

Never use . Any styling is up to a stylesheet.

chipx86chipx86

Never use , for the same reason as .

chipx86chipx86

Trans blocks for the strings in here.

mike_conleymike_conley

Don't use . Should use .

chipx86chipx86

Why do you need this? It feels wrong having it.

chipx86chipx86

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Two things URLs should always have: 1) A trailing / 2) A name= parameter.

chipx86chipx86

Should be separated by a blank line.

chipx86chipx86

No blank line.

chipx86chipx86

Would you mind maybe putting a "_ajax" suffix on any functions like this? Actually though, I think this function would …

chipx86chipx86

No blank line.

chipx86chipx86

We do this more than once. Best to pull it out in the beginning.

chipx86chipx86

typo: sanitizing

mike_conleymike_conley

No blank line.

chipx86chipx86

typo: obtaining

mike_conleymike_conley

A good thing of polish would be to change these error strings. They should be more human-readable, less technical, and …

chipx86chipx86

This is kind of funky. I'd suggest getting the extension manager first, then operating on it. If you still need …

chipx86chipx86

Remove the blank line.

chipx86chipx86

Best to use format strings.

chipx86chipx86

Did it? If we catch an Exception on lines 57-58, we'll store the error string in response, but the status …

mike_conleymike_conley

Remove the blank line.

chipx86chipx86

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Some of the same comments.

chipx86chipx86

Go through and apply the same blank line removal as the previous change. Blank lines are good, but there shouldn't …

chipx86chipx86

This should default to None. You can override it in your own settings_local.py.

chipx86chipx86

As Mike mention, this should be a .less file. Also make sure ou are following our style. At a quick …

chipx86chipx86

"Listener" Same elsewhere. Note that you should be using 4 space indentation (no tabs!) everywhere.

chipx86chipx86

Space before { Same throughout the file. Also, you're fetching .item_actions more than once. This will traverse the entire DOM. …

chipx86chipx86

jQuery does chaining, so you can do: $('#' + id) .hide() .after(...) Note that you shouldn't be building HTML though.

chipx86chipx86

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Should have a trailing slash after "browser" I would change "browser" to "browse" in the URL, though. Also, this is …

chipx86chipx86

reviewboard.extensionbrowser isn't being recognized as a module because there's no __init__.py under reviewboard/extensionbrowser. You probably have one locally, but haven't …

mike_conleymike_conley

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (95 > 79 characters)

reviewbotreviewbot

"Keywords" should maybe be wrapped in an underscore function. Also, not sure "Keywords" is the right word to use here. …

mike_conleymike_conley

"Search" should be in a trans tag

mike_conleymike_conley

Mind if I ask why a wasn't used here instead? I know we didn't want it to *look* like a …

mike_conleymike_conley

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 8 W292 no newline at end of file

reviewbotreviewbot

We use 2 spaces indentation in our .css, and I think you've used tabs. Also, please put your properties in …

mike_conleymike_conley

id:, as opposed to id :

mike_conleymike_conley

Space before {

mike_conleymike_conley

Please remove this log

mike_conleymike_conley

Modals are not usually the best way to handle things. We have a thing that drops down from the top …

mike_conleymike_conley

Space before { after function() - true for the rest of these Javascript files too.

mike_conleymike_conley

I wonder if instead of tossing this into the view, we render this as a hidden node in the page. …

mike_conleymike_conley

remove the newline

mike_conleymike_conley

remove the newline

mike_conleymike_conley

No newline.

mike_conleymike_conley

Yes please. :)

mike_conleymike_conley

See my other comment, re the "Loading" banner.

mike_conleymike_conley

No newline.

mike_conleymike_conley

var targets = ["ext-information", "ext-screenshots", etc]; is preferable over new Array.

mike_conleymike_conley

Again, I wonder if these should be in the rendered HTML instead, in something like this: ... Instead, for translation, …

mike_conleymike_conley

[] as opposed to new Array. Also, this looks related to the array on line 91. If we end up …

mike_conleymike_conley

Col: 35 W291 trailing whitespace

reviewbotreviewbot

These should be in alphabetical order, so DOES_NOT_EXIST, EXTENSION_INSTALLED, INVALID_FORM_DATA, NOT_LOGGED_IN, PERMISSION_DENIED.

mike_conleymike_conley

EXTENSION_INSTALLED does not seem to exist in either this patch, or in the patch in /r/3973

mike_conleymike_conley

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Needs documentation - see ServerInfoResource, for example.

mike_conleymike_conley

Needs documentation. I guess that's what this handy newline is supposed to indicate. :)

mike_conleymike_conley

Might be worth logging the error details too.

mike_conleymike_conley

Needs documentation

mike_conleymike_conley

Needs documentation

mike_conleymike_conley

Col: 21 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 21 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 17 E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

We should probably log this error.

mike_conleymike_conley

Does this need to get wrapped in an underscore translation function?

mike_conleymike_conley

We should definitely log this, and maybe throw a new INSTALL_EXTENSION_FAILED error?

mike_conleymike_conley

Documentation needed.

mike_conleymike_conley

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 57 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 59 W291 trailing whitespace

reviewbotreviewbot

Col: 13 E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 52 E502 the backslash is redundant between brackets

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 53 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 83 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 61 W291 trailing whitespace

reviewbotreviewbot

Col: 53 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 13 E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

[comment deleted]

mike_conleymike_conley

[comment deleted]

mike_conleymike_conley

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

On attempting a search with this latest patch, I'm getting this trace: http://pastie.org/7308158 Apparently, json.JSONDecodeError doesn't exist for my version …

mike_conleymike_conley

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

[comment deleted]

mike_conleymike_conley

[comment deleted]

mike_conleymike_conley

Col: 53 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Trailing whitespace.

mike_conleymike_conley

Trailing whitespace

mike_conleymike_conley

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Trailing whitespace

mike_conleymike_conley

var TabbedModalBoxView

mike_conleymike_conley

typo: install -> installs

mike_conleymike_conley

var InfoView

mike_conleymike_conley

Trailing whitespace

mike_conleymike_conley

Nit - indent one more space.

mike_conleymike_conley

Swap INVALID_FORM_DATA and INSTALL_EXTENSION_FAILED.

mike_conleymike_conley

Col: 13 E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

Can we pull this out into its own patch? I'd like to get it into a release. Also, I think …

chipx86chipx86

No blank line here.

chipx86chipx86

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

We should rename 'id' to something else. id is a reserved keyword.

chipx86chipx86

These blank lines should be removed.

chipx86chipx86

Blank line before this.

chipx86chipx86

There are alignment problems here.

chipx86chipx86

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 45 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Comma after "dependencies"

chipx86chipx86

Blank line before.

chipx86chipx86

Generally we'll do: return { 'error': ..., }

chipx86chipx86

Kind of awkwardly formatted. I'd prefer: response[1][....] = \ package_name in self.installed_extension

chipx86chipx86

Col: 53 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Space before the "/>"

chipx86chipx86

IDs like "results" make me nervous. Far too generic. Prefixing a lot of these IDs with something to make them …

chipx86chipx86

These should be indented relative to other template tags. They're too far indented right now. Same with the other nearby …

chipx86chipx86

Missing {% trans %} tags.

chipx86chipx86

You shouldn't need the "./" part.

chipx86chipx86

Blank line before this.

chipx86chipx86

Indent "compressed_js" one space.

chipx86chipx86

Col: 5 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Blank line before this.

chipx86chipx86

Blank line before this.

chipx86chipx86

No blank line here.

chipx86chipx86

Should be formatted like: params = { 'query': ..., 'offset': ..., }

chipx86chipx86

Blank line before this.

chipx86chipx86

Comma after.

chipx86chipx86

Swap these. Alphabetical order.

chipx86chipx86

Should be nesting these better in the li above, like: li { ... &.header div { ... } div { …

chipx86chipx86

Can this be removed now?

chipx86chipx86

The name should be a bit more specific, name-wise. I'd also feel better with this file not being here in …

chipx86chipx86

Defaults goes first.

chipx86chipx86

This is an unsafe assumption. You should use: SITE_ROOT + "api/..."

chipx86chipx86

No blank line here.

chipx86chipx86

Only one model per file. This needs to be in a separate file.

chipx86chipx86

defaults goes first.

chipx86chipx86

Same comment as above.

chipx86chipx86

Why's this blank?

chipx86chipx86

No blank line.

chipx86chipx86

Remove this. We get it by default.

chipx86chipx86

I'm not sure we need this. I'd prefer not using _bindAll unless we have specific uses. In general, we should …

chipx86chipx86

As in my previous comment, this should be using _.template, and joining an array of lines.

chipx86chipx86

One var statement per function, at the top of the function, with each parameter being on its own line, comma-separated. …

chipx86chipx86

Should be: this.$el .html(template) .modalBox(); As for the class, that should go in the className field at the top of …

chipx86chipx86

Should be: new InfoView({ model: this.model })

chipx86chipx86

No blank line.

chipx86chipx86

Each template should be compiled here, once, and not later.

chipx86chipx86

Remove this. We get it by default.

chipx86chipx86

Same comment as above.

chipx86chipx86

render shouldn't take an ID. I think if we have this one thing representing different tabs, we should really just …

chipx86chipx86

Should be done inline above, so it's only ever done once per page.

chipx86chipx86

this.$el

chipx86chipx86

A view should never pack itself. That's up to the code creating and rendering the class.

chipx86chipx86

The dictionary fields should be indented 1 level from the "var extension_install". Also, extension_install should be extensionInstallInfo. (No underscores in …

chipx86chipx86

No need for the 'self'. Instead, pass 'this' as the second parameter to save(), which will bind the proper context.

chipx86chipx86

Alphabetical order.

chipx86chipx86

No blank line.

chipx86chipx86

This is too generic, given the context. This file's too big. I'd recommend we start breaking this out. Maybe put …

chipx86chipx86

Summaries can only be one line.

chipx86chipx86

There should be a has_access_permissions function that does this, and the get function should call that.

chipx86chipx86

Put package_name in the parameter list for the function, and then pass to get_links below.

chipx86chipx86

Blank line before this.

chipx86chipx86

Not enough context in the log statement.

chipx86chipx86

Should be removed.

chipx86chipx86

The fields should be on their own lines, just like other resources.

chipx86chipx86

This shouldn't be able to work. It will turn into ('P', 'O', 'S', 'T'). Make sure it's ('POST',)

chipx86chipx86

Same comment as above.

chipx86chipx86

Alignment issue. The _entrypoint_iterator() is private and shouldn't be used. There'd need to be an accessor.

chipx86chipx86

Col: 13 E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

The wrapping is weird. You're calling get_extension_manager() a lot. Call it once and just use that variable.

chipx86chipx86

There needs to be more context. I think, though, that install_extension should be logging.

chipx86chipx86

Summaries can only be one line.

chipx86chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/admin/forms.py
        reviewboard/settings.py
        reviewboard/extensionbrowser/base.py
        reviewboard/extensionbrowser/forms.py
        reviewboard/extensionbrowser/views.py
        reviewboard/urls.py
        reviewboard/extensionbrowser/urls.py
      Ignored Files:
        reviewboard/extensionbrowser/templates/extensionbrowser/extension_browser.html
        reviewboard/static/rb/js/extensionbrowser.js
        reviewboard/extensionbrowser/templates/extensionbrowser/extension_details.html
        reviewboard/static/rb/css/extensionbrowser.css
    
    
  2. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/extensionbrowser/urls.py (Diff revision 1)
     
     
    Show all issues
    Col: 5
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  5. reviewboard/urls.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  6. 
      
mike_conley
  1. Surya:
    
    This is really coming along nicely. Just some high-level suggestions / notes, along with some typos I noticed.
    
    There are a few style things also worth fixing, but we'll bring those up when you're out of WIP.
    
    Let me know if you have any questions. Thanks,
    
    -Mike
  2. reviewboard/admin/forms.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    Er, isn't this a change from one of your original easyfix bugs?
  3. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Just curious - why are we making extension_manager optional?
    1. Recent changes, changed that. So it's no longer optional.
  4. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    typo: corresponding
    1. Changed the structure of the method, thus changing the docstring.
  5. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    If an extension manager wasn't passed to the constructor, self.installed_extensions will be None, which is not iterable.
  6. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    typo: package
  7. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    Wouldn't we want to pass the error on to the caller instead of swallowing it silently?
  8. Show all issues
    All of the strings in here should be wrapped in trans blocks
  9. Show all issues
    Trans blocks for the strings in here.
  10. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
    Show all issues
    typo: sanitizing
  11. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
    Show all issues
    typo: obtaining
  12. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
     
    Show all issues
    Did it? If we catch an Exception on lines 57-58, we'll store the error string in response, but the status will be 204? That doesn't sound right.
  13. You might want to check out how we use LessCSS to compile our CSS and use things like variables to use common colours, etc.
  14. 
      
chipx86
  1. This is looking cool, but there's a lot of stylistic stuff to fix up, and some more important architectural stuff.
    
    We're moving all our JavaScript to be Model/View based, using Backbone.js. I want to see all new JavaScript fit this. Start thinking in terms of objects that contain data/core logic, and objects that render (using templates, building jQuery elements, etc.).
    
    You can look at some of our other code to see how we're doing this, and of course talk to us if you need help structuring it.
  2. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    Imports are always in up to 3 groups:
    
    1) Python-bundled modules
    2) Third-party modules (like simplejson)
    3) Project's modules (like reviewboard)
    
    Separated by blank lines.
    
    Also, never import from reviewboard.settings. This should be using:
    
        from django.conf import settings
    
    They're too very different things. One is our settings file, another is a wrapper that may be modified dynamically (by siteconfig).
    1. For the last part "from django.conf import settings" if I change it to that, I can no longer access EXTENSION_STORE_ENDPOINT. There's an import error?
    2. What does that error look like? It certainly shouldn't be a problem, unless something is messed up (somehow grabbing the wrong settings file, for example).
    3. It's fixed now, it was an error on my part. Sorry about that.
  3. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    Shouldn't use simplejson. Instead, for now, use django.utils.simplejson. (This is going away, but let's be consistent for now.)
  4. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    We now have multiple things named Extension. Let's change this one to StoreExtensionInfo.
  5. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    This is very technical and too specific to the current workflow. This level of code should not know how it will be used (as that may change). Instead, it should just say what it is and what, in general, it's meant to represent.
  6. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    This is the kind of thing that should be handled by the rendering, not core logic. I'd say just nuke this function and do this in the template.
  7. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
     
    Show all issues
    The JSON response part is an implementation detail, and not something that needs to be known to consumers of the API.
  8. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
     
     
    Show all issues
    Remove the blank line.
  9. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
     
    Show all issues
    Generally when a list comprehension spans multiple lines, we like to do:
    
        self.foo = [
            ext.dist.project_name.lower()
            for ext in ....
        ]
  10. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    Use format strings. They're cheaper for Python.
  11. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    Same.
    
    In fact, combine this with the previous line, too.
    1. Changed it to format strings, but can't combine. It needs to append params only when they're supplied.
  12. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    What happens when/if this fails?
    1. Since we would be dealing with the response from the store eventually, do we still need to make sure that the JSON is valid and doesn't break the parsing?
    2. Yes, since the server could be having a problem and returning some error page or something, or everything is shut down temporarily for maintenance, or an HTTP proxy is being weird, or someone's testing at an Internet Cafe or hotel and get that annoying "Confirm the Terms of Service" page for wifi, or a dozen other things.
  13. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    Make sure to change the "Extension" part when renaming the class.
  14. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    Since we're using a dictionary, does this mean we can't rely on any certain ordering from the server?
    1. Yes, that's a problem I suppose. Especially when this would be extended to using sorted results from the store. Fixed it to using lists instead.
  15. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
     
     
    Show all issues
    Given how many arguments we have, and that this will likely grow, can you instead use keyword arguments for this? And then wrap the values so they're indented 1 level from the previous line, with one keyword argument per line.
  16. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
    Show all issues
    It may not be a zip file. It could be an egg, or tar.gz, or whatever. Best not to be specific here.
    1. Can you comment on the issues that were dropped so that there's a record of why they were dropped?
    2. Sure.
  17. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    We're repeating the whole HTTP code a couple times. Can this be consolidated into a wrapper function?
    1. Removed, was no longer needed with the webAPI/Backbone change.
  18. reviewboard/extensionbrowser/base.py (Diff revision 1)
     
     
     
     
    Show all issues
    One-line summaries shouldn't have both """ on the same line.
    
    In this function, though, there's no documentation on what this extension information is.
    1. I can write about all the fields that is parsed from the JSON response so that what "information" is retrieved is know, but it would get long. Is that fine?
    2. Just a summary of what sort of data we're fetching. Don't need too much detail. Right now, it's just too generic.
  19. reviewboard/extensionbrowser/forms.py (Diff revision 1)
     
     
    Show all issues
    Shouldn't have the ":" in the label. That's a rendering detail.
  20. Show all issues
    See about using {% static %} for this, since {% admin_media_prefix %} is going away.
    1. How do I route to the admin_media directory using {% static %}?
    2. You should be able to just do {% static "admin/css/forms.css" %}.
  21. Show all issues
    Shouldn't use either of these. Instead, use {% compressed_css %} with some name, which you'll register in settings.py in PIPELINE_CSS. This will list all the CSS files you use.
    
    This ties into our build system, so that 3 files can be turned into one minified file.
  22. Show all issues
    Should be scripts-post, so that we load this after rendering the page.
  23. Show all issues
    Blank line before this.
  24. Do we not have one of these already? (Not sure, asking.)
  25. Show all issues
    Always use {% url %} instead of hard-coding URLs.
  26. Show all issues
    Indentation is screwy.
    1. Could you please explain how to fix that? Shouldn't the div be idented within the if?
    2. It can seem a bit weird, but we have two independent levels of indentation.
      
      One is indentation of template tags. Template tags should be indented (within the {% .. %}) with respect to other template tags, but not to HTML tags.
      
      The other is HTML tags. Regardless of template tags, the HTML tags should be indented relative to other HTML tags.
      
      So in the above, the </form> and <div> should be aligned.
  27. Show all issues
    Use {% trans %}
  28. Show all issues
    Any IDs we don't need should be removed.
    
    Any we do should be properly namespaced.
  29. Show all issues
    Use {% trans %} for all these strings.
    
    Go through the files and make sure you're always localizing.
  30. Show all issues
    Seem to have some tab vs. space problems. Make sure this file has no tabs, and no trailing whitespace.
  31. Show all issues
    Classes always use -, not _
  32. Show all issues
    Never use <center>. Any styling is up to a stylesheet.
    1. File doesn't exist anymore.
  33. Show all issues
    Never use <b>, for the same reason as <center>.
    1. File has been removed.
  34. Show all issues
    Don't use <b>. Should use <label>.
    1. File has been removed.
  35. Show all issues
    Why do you need this? It feels wrong having it.
    1. How do I go about clearing the floats otherwise?
    2. So our codebase, for legacy reasons, does do class="clearfix" (which clears things), but clearfix is done on the element with the floats. It will clear after the element.
      
      What we probably should do is import it into the proper CSS rule instead, by putting the following in that rule:
      
          .clearfix
      
      (or is it .clearfix() ? It's late.. I don't remember. One of those.)
      
      This might require that .clearfix move into defs.less.
  36. reviewboard/extensionbrowser/urls.py (Diff revision 1)
     
     
     
     
    Show all issues
    Two things URLs should always have:
    
    1) A trailing /
    2) A name= parameter.
  37. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
     
    Show all issues
    Should be separated by a blank line.
    1. Doesn't exist anymore.
  38. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
     
    Show all issues
    No blank line.
  39. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
    Show all issues
    Would you mind maybe putting a "_ajax" suffix on any functions like this?
    
    Actually though, I think this function would be best done as an API resource. Maybe a next step on top of this change.
    
    That way, we can continue to use our API as the one true way to talk to Review Board for most things, and admins can script installation of extensions.
  40. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
     
     
    Show all issues
    No blank line.
  41. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
    Show all issues
    We do this more than once. Best to pull it out in the beginning.
  42. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
     
    Show all issues
    No blank line.
  43. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
    Show all issues
    A good thing of polish would be to change these error strings. They should be more human-readable, less technical, and say roughly what went wrong, maybe even what to do about it.
  44. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
     
    Show all issues
    This is kind of funky.
    
    I'd suggest getting the extension manager first, then operating on it.
    
    If you still need to wrap, wrap at the argument level.
  45. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
     
     
    Show all issues
    Remove the blank line.
  46. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
    Show all issues
    Best to use format strings.
  47. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
     
    Show all issues
    Remove the blank line.
  48. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
    Show all issues
    Some of the same comments.
  49. reviewboard/extensionbrowser/views.py (Diff revision 1)
     
     
     
     
    Show all issues
    Go through and apply the same blank line removal as the previous change.
    
    Blank lines are good, but there shouldn't be one in the following cases:
    
    1) At the start of a function.
    2) Before an else/elif block.
  50. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
    This should default to None. You can override it in your own settings_local.py.
  51. Show all issues
    As Mike mention, this should be a .less file.
    
    Also make sure ou are following our style. At a quick glance:
    
    1) Always have a space before {
    2) Indent with 2 spaces.
  52. Show all issues
    "Listener"
    
    Same elsewhere.
    
    Note that you should be using 4 space indentation (no tabs!) everywhere.
  53. Show all issues
    Space before {
    
    Same throughout the file.
    
    Also, you're fetching .item_actions more than once. This will traverse the entire DOM.
    
    Best to fetch that only once (and within some known parent element), and then fetch whatever else you need from it.
    1. Can I store the nodes from a search? like var x = $('.item_actions'); and use x multiple times?
    2. Yep, that's the way to do it!
      
      And if you know of a base element with an ID or some existing element handle that .item-actions (should be -, not _) is in, then you can narrow it down further (thus speeding things up) by doing myel.find('.item-actions').
  54. Show all issues
    jQuery does chaining, so you can do:
    
    $('#' + id)
        .hide()
        .after(...)
    
    Note that you shouldn't be building HTML though.
  55. reviewboard/urls.py (Diff revision 1)
     
     
    Show all issues
    Should have a trailing slash after "browser"
    
    I would change "browser" to "browse" in the URL, though.
    
    Also, this is longer than 80 chars, so wrap the include to the next line.
  56. 
      
AY
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
        reviewboard/webapi/resources.py
        reviewboard/extensionbrowser/base.py
        reviewboard/extensionbrowser/forms.py
        reviewboard/extensionbrowser/views.py
        reviewboard/urls.py
        reviewboard/extensionbrowser/urls.py
      Ignored Files:
        reviewboard/static/rb/css/images/ui-icons_2e83ff_256x240.png
        reviewboard/static/rb/css/images/ui-bg_glass_95_fef1ec_1x400.png
        reviewboard/static/rb/css/images/ui-icons_469bdd_256x240.png
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_fcfdfd_1x100.png
        reviewboard/static/rb/css/images/ui-icons_f9bd01_256x240.png
        reviewboard/static/rb/css/extensionbrowser.css
        reviewboard/static/rb/css/images/ui-icons_217bc0_256x240.png
        reviewboard/static/rb/css/images/ui-icons_d8e7f3_256x240.png
        reviewboard/static/rb/css/jquery-ui-1.10.1.custom.css
        reviewboard/extensionbrowser/templates/extensionbrowser/extension_browser.html
        reviewboard/static/rb/js/models/extensionInformationModel.js
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_f5f8f9_1x100.png
        reviewboard/static/rb/js/extensionbrowser.js
        reviewboard/static/rb/css/images/ui-bg_glass_85_dfeffc_1x400.png
        reviewboard/static/rb/css/images/ui-icons_6da8d5_256x240.png
        reviewboard/static/rb/css/images/ui-icons_cd0a0a_256x240.png
        reviewboard/static/rb/css/images/ui-bg_flat_55_fbec88_40x100.png
        reviewboard/static/rb/css/images/ui-bg_gloss-wave_55_5c9ccc_500x100.png
        reviewboard/static/rb/js/views/extensionBrowserView.js
        reviewboard/static/rb/css/images/ui-bg_flat_0_aaaaaa_40x100.png
        reviewboard/static/rb/css/images/animated-overlay.gif
        reviewboard/static/rb/css/images/ui-bg_glass_75_d0e5f5_1x400.png
    
    
  2. reviewboard/extensionbrowser/base.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/extensionbrowser/base.py (Diff revision 2)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/extensionbrowser/base.py (Diff revision 2)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/extensionbrowser/base.py (Diff revision 2)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/extensionbrowser/base.py (Diff revision 2)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/extensionbrowser/base.py (Diff revision 2)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/extensionbrowser/base.py (Diff revision 2)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/extensionbrowser/base.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (95 > 79 characters)
    
  10. reviewboard/extensionbrowser/urls.py (Diff revision 2)
     
     
    Show all issues
    Col: 5
     E128 continuation line under-indented for visual indent
    
  11. reviewboard/extensionbrowser/views.py (Diff revision 2)
     
     
    Show all issues
    Col: 8
     W292 no newline at end of file
    
  12. reviewboard/urls.py (Diff revision 2)
     
     
    Show all issues
    Col: 35
     W291 trailing whitespace
    
  13. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  14. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Col: 21
     E126 continuation line over-indented for hanging indent
    
  15. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Col: 21
     E126 continuation line over-indented for hanging indent
    
  16. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Col: 17
     E123 closing bracket does not match indentation of opening bracket's line
    
  17. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  18. 
      
mike_conley
  1. 
      
  2. Show all issues
    One thing that strikes me is that when you switch from tab to tab, the size of the modal changes, and so the tabs move.
    
    This is not ideal. What we should do is size the modal to fit the largest tab content, and stick to that size, so that we're not resizing on the fly.
    1. Added a fixed height to the tabs.
    1. Details as in why/what each dependency is needed for? As in add more information?
    2. The way I interpreted that is that the tab itself should be flush up against the content pane.
  3. reviewboard/extensionbrowser/base.py (Diff revision 2)
     
     
    Show all issues
    reviewboard.extensionbrowser isn't being recognized as a module because there's no __init__.py under reviewboard/extensionbrowser. You probably have one locally, but haven't included it. You need to include it.
    1. It's being staged (git status doesn't show me any new files) but I'm not sure why it isn't showing up here. Perhaps is it because it's a blank file? How do I force commit?
    2. Yeah, that's a bit of a Review Board wart. Empty files are considered unchanged and aren't shown. We'll have to remember to make one..
  4. reviewboard/extensionbrowser/forms.py (Diff revision 2)
     
     
    Show all issues
    "Keywords" should maybe be wrapped in an underscore function.
    
    Also, not sure "Keywords" is the right word to use here. Makes me think of AOL. Let's leave it for now, but maybe worth revisiting later on.
    1. I changed it to "Search" (think you pointed this out earlier on the IRC as well and recommended to change it to "Search").
      
      Also just making sure, the underscore is from - from django.utils.translation import ugettext as _?
    2. Yep.
  5. Show all issues
    "Search" should be in a trans tag
  6. Show all issues
    Mind if I ask why a <table> wasn't used here instead? I know we didn't want it to *look* like a table, but a table, semantically speaking, is what it is...
    1. ChipX86 suggested that I adopt a table based design but not a table since he wanted it to be fluid (more on this below).
    2. Don't worry too much about this. I think by the time we ship it, we're going to do a full UI rethink in order to get it to match whatever the website will end up being.
  7. Show all issues
    We use 2 spaces indentation in our .css, and I think you've used tabs.
    
    Also, please put your properties in alphabetical order, and always make sure there is a space before {.
    
    I'm a bit worried that some of these hard-coded widths and heights might be a little fragile...
    
    Also, it might be worth looking into converting this into a .less file. See http://lesscss.org/
    1. Changed the spacing. I've most of the widths percentages (except for the some of the parents) so it should be mostly fluid now.
      
      Ported some duplications to .less, if I can make it even better please let me know.
  8. Show all issues
    id:, as opposed to id :
  9. Show all issues
    Space before {
  10. Show all issues
    Please remove this log
  11. Show all issues
    Modals are not usually the best way to handle things. We have a thing that drops down from the top of the screen that says "Loading" when doing an AJAX request, and if something goes wrong, that Loading banner becomes an error that stays put. Maybe use that instead.
    1. That's exactly what I'm looking to do as well. But I'm wondering how to do it. When I was using RB.apiCall() it showed up the loader on the top-center of the screen. If something went wrong, a modal box with an error showed up. Is there any convenience method that does it or should I make my own global error modal and pass it the error message?
    2. We don't have a great way to do errors right now. Go ahead and use alert for now. Some other code does too. I want to figure this out in 1.8 while we do the JS rework.
  12. Show all issues
    Space before { after function() - true for the rest of these Javascript files too.
  13. reviewboard/static/rb/js/views/extensionBrowserView.js (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    I wonder if instead of tossing this into the view, we render this as a hidden node in the page. That'd give us trans tags too.
    1. The idea of having these nodes being re-populated everytime seemed inefficient at first as well, but since this node is within a modalbox() call, the node is nuked from the DOM when the modalbox is closed. So I have to populate the contents of modalbox each time I invoke it.
    2. Eventually we'll be able to put this into a template file within the js/ directory and have require.js bundle it sanely.
      
      We're going to need to provide localization properly for JavaScript soon. Django supports much of this. We just need to start using it.
      
      Btw, for templates like this, it's best to define the template only once, inside the class's prototype (not in a function, but on the same level as functions/constants). Instead of one big string, you want to use an array of strings, one per line, and then join them (it's faster). You can look at other views for how to do this.
    3. Last two lines of optimization is now implemented.
  14. Show all issues
    remove the newline
  15. Show all issues
    remove the newline
  16. Show all issues
    No newline.
  17. Show all issues
    Yes please. :)
  18. Show all issues
    See my other comment, re the "Loading" banner.
    1. Pending fix: See comment above.
  19. Show all issues
    No newline.
  20. Show all issues
    var targets = ["ext-information", "ext-screenshots", etc];
    
    is preferable over new Array.
  21. Show all issues
    Again, I wonder if these should be in the rendered HTML instead, in something like this:
    
    <script type="text/template" id="itemViewTemplate">
      <div class="extension_info_wrapper">
        ...
      </div>
    </script>
    
    Instead, for translation, and to keep our HTML (even our templates) a little more separated from our JS.
    1. I initially thought/proposed the same with the text/template. But I asked ChipX86 and he suggessted that for now these templates remain within the JS files. Eventually, ReviewBoard will use require.js to load in the templates from external files (like the way django does) and parse them.
    2. Yep. See my comment above.
  22. Show all issues
    [] as opposed to new Array.
    
    Also, this looks related to the array on line 91.
    
    If we end up going with the embedded template route, maybe this object have an object property, like:
    
    target_templates: {
      'ext-information': 'info_template',
      'ext-screenshots': 'screenshots'
    }
    
    etc, where 'info_template' and 'screenshots' could be the template IDs. Just an idea, to reduce duplication.
    1. I'd still need integer based indexing. I looked into this, but the tabs() component of the UI returns an integer (when the tab is changed) so the lookup has to be integer based.
    2. Yeah... tabs in jquery-UI are a pain.
      
      I don't remember, isn't there a way to get the class of the current tab, though? At the very least, it should be possible to do a query for it, and then compare class names/IDs.
    3. So there's a way to get the target ID from the UI object, so I made changes to the code and templates are now objects.
  23. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
     
    Show all issues
    These should be in alphabetical order, so DOES_NOT_EXIST, EXTENSION_INSTALLED, INVALID_FORM_DATA, NOT_LOGGED_IN, PERMISSION_DENIED.
  24. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    EXTENSION_INSTALLED does not seem to exist in either this patch, or in the patch in /r/3973
    1. Will be up on /r/3973 update shortly.
  25. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Needs documentation - see ServerInfoResource, for example.
  26. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Needs documentation. I guess that's what this handy newline is supposed to indicate. :)
  27. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Might be worth logging the error details too.
  28. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Needs documentation
  29. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Needs documentation
  30. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    We should probably log this error.
  31. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Does this need to get wrapped in an underscore translation function?
    1. I'm not sure, I looked up how other resources handle the INVALID_FORM_DATA error and they seem to returning plaintext as is.
    2. Currently we don't. Probably should.
  32. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    We should definitely log this, and maybe throw a new INSTALL_EXTENSION_FAILED error?
  33. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Documentation needed.
  34. 
      
AY
AY
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
        reviewboard/webapi/resources.py
        reviewboard/__init__.py
        reviewboard/extensionbrowser/base.py
        reviewboard/extensionbrowser/forms.py
        reviewboard/extensionbrowser/views.py
        reviewboard/urls.py
        reviewboard/extensionbrowser/urls.py
      Ignored Files:
        reviewboard/static/rb/css/images/ui-icons_2e83ff_256x240.png
        reviewboard/static/rb/css/images/ui-bg_glass_95_fef1ec_1x400.png
        reviewboard/static/rb/css/images/ui-icons_469bdd_256x240.png
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_fcfdfd_1x100.png
        reviewboard/static/rb/css/images/ui-icons_f9bd01_256x240.png
        reviewboard/static/rb/css/extensionbrowser.less
        reviewboard/static/rb/css/images/ui-icons_d8e7f3_256x240.png
        reviewboard/static/rb/css/jquery-ui-1.10.1.custom.css
        reviewboard/extensionbrowser/templates/extensionbrowser/extension_browser.html
        reviewboard/static/rb/js/models/extensionInformationModel.js
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_f5f8f9_1x100.png
        reviewboard/static/rb/js/extensionbrowser.js
        reviewboard/static/rb/css/images/ui-bg_flat_0_aaaaaa_40x100.png
        reviewboard/static/rb/css/images/ui-bg_glass_85_dfeffc_1x400.png
        reviewboard/static/rb/css/images/ui-icons_6da8d5_256x240.png
        reviewboard/static/rb/css/images/ui-icons_cd0a0a_256x240.png
        reviewboard/static/rb/css/images/ui-bg_flat_55_fbec88_40x100.png
        reviewboard/static/rb/css/images/ui-bg_gloss-wave_55_5c9ccc_500x100.png
        reviewboard/static/rb/css/images/ui-bg_glass_75_d0e5f5_1x400.png
        reviewboard/static/rb/css/images/ui-icons_217bc0_256x240.png
        reviewboard/static/rb/css/images/animated-overlay.gif
        reviewboard/static/rb/js/views/extensionBrowserView.js
    
    
  2. reviewboard/__init__.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  3. reviewboard/extensionbrowser/base.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  4. reviewboard/extensionbrowser/base.py (Diff revision 3)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/extensionbrowser/base.py (Diff revision 3)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/extensionbrowser/base.py (Diff revision 3)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/extensionbrowser/base.py (Diff revision 3)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/extensionbrowser/base.py (Diff revision 3)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/extensionbrowser/base.py (Diff revision 3)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  10. reviewboard/extensionbrowser/base.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  11. reviewboard/extensionbrowser/base.py (Diff revision 3)
     
     
    Show all issues
    Col: 57
     E127 continuation line over-indented for visual indent
    
  12. reviewboard/extensionbrowser/urls.py (Diff revision 3)
     
     
    Show all issues
    Col: 5
     E128 continuation line under-indented for visual indent
    
  13. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  14. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    Col: 59
     W291 trailing whitespace
    
  15. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    Col: 13
     E123 closing bracket does not match indentation of opening bracket's line
    
  16. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  17. 
      
AY
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
        reviewboard/webapi/resources.py
        reviewboard/extensionbrowser/views.py
        reviewboard/__init__.py
        reviewboard/extensionbrowser/forms.py
        reviewboard/extensionbrowser/base.py
        reviewboard/urls.py
        reviewboard/extensionbrowser/urls.py
      Ignored Files:
        reviewboard/static/rb/css/images/ui-icons_2e83ff_256x240.png
        reviewboard/static/rb/css/images/ui-bg_glass_95_fef1ec_1x400.png
        reviewboard/static/rb/css/images/ui-icons_469bdd_256x240.png
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_fcfdfd_1x100.png
        reviewboard/static/rb/css/images/ui-icons_f9bd01_256x240.png
        reviewboard/static/rb/css/extensionbrowser.less
        reviewboard/static/rb/css/images/ui-icons_d8e7f3_256x240.png
        reviewboard/static/rb/css/jquery-ui-1.10.1.custom.css
        reviewboard/extensionbrowser/templates/extensionbrowser/extension_browser.html
        reviewboard/static/rb/js/models/extensionInformationModel.js
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_f5f8f9_1x100.png
        reviewboard/static/rb/js/extensionbrowser.js
        reviewboard/static/rb/css/images/ui-bg_flat_0_aaaaaa_40x100.png
        reviewboard/static/rb/css/images/ui-bg_glass_85_dfeffc_1x400.png
        reviewboard/static/rb/css/images/ui-icons_6da8d5_256x240.png
        reviewboard/static/rb/css/images/ui-icons_cd0a0a_256x240.png
        reviewboard/static/rb/css/images/ui-bg_flat_55_fbec88_40x100.png
        reviewboard/static/rb/css/images/ui-bg_gloss-wave_55_5c9ccc_500x100.png
        reviewboard/static/rb/css/images/ui-bg_glass_75_d0e5f5_1x400.png
        reviewboard/static/rb/css/images/ui-icons_217bc0_256x240.png
        reviewboard/static/rb/css/images/animated-overlay.gif
        reviewboard/static/rb/js/views/extensionBrowserView.js
    
    
  2. reviewboard/__init__.py (Diff revision 4)
     
     
    Show all issues
    Col: 52
     E502 the backslash is redundant between brackets
    
  3. reviewboard/extensionbrowser/base.py (Diff revision 4)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  4. reviewboard/extensionbrowser/base.py (Diff revision 4)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/extensionbrowser/base.py (Diff revision 4)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/extensionbrowser/base.py (Diff revision 4)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/extensionbrowser/base.py (Diff revision 4)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/extensionbrowser/base.py (Diff revision 4)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/extensionbrowser/base.py (Diff revision 4)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  10. reviewboard/extensionbrowser/base.py (Diff revision 4)
     
     
    Show all issues
    Col: 53
     E127 continuation line over-indented for visual indent
    
  11. reviewboard/extensionbrowser/urls.py (Diff revision 4)
     
     
    Show all issues
    Col: 5
     E128 continuation line under-indented for visual indent
    
  12. reviewboard/webapi/resources.py (Diff revision 4)
     
     
    Show all issues
    Col: 13
     E123 closing bracket does not match indentation of opening bracket's line
    
  13. 
      
AY
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
        reviewboard/webapi/resources.py
        reviewboard/extensionbrowser/views.py
        reviewboard/__init__.py
        reviewboard/extensionbrowser/forms.py
        reviewboard/extensionbrowser/base.py
        reviewboard/urls.py
        reviewboard/extensionbrowser/urls.py
      Ignored Files:
        reviewboard/static/rb/css/images/ui-icons_2e83ff_256x240.png
        reviewboard/static/rb/css/images/ui-bg_glass_95_fef1ec_1x400.png
        reviewboard/static/rb/css/images/ui-icons_469bdd_256x240.png
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_fcfdfd_1x100.png
        reviewboard/static/rb/css/images/ui-icons_f9bd01_256x240.png
        reviewboard/static/rb/css/extensionbrowser.less
        reviewboard/static/rb/css/images/ui-icons_d8e7f3_256x240.png
        reviewboard/static/rb/css/common.less
        reviewboard/extensionbrowser/templates/extensionbrowser/extension_browser.html
        reviewboard/static/rb/js/models/extensionInformationModel.js
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_f5f8f9_1x100.png
        reviewboard/static/rb/js/extensionbrowser.js
        reviewboard/static/rb/css/jquery-ui-1.7.1.custom.css
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/images/ui-bg_flat_0_aaaaaa_40x100.png
        reviewboard/static/rb/css/images/ui-bg_glass_85_dfeffc_1x400.png
        reviewboard/static/rb/css/images/ui-icons_6da8d5_256x240.png
        reviewboard/static/rb/css/images/ui-icons_cd0a0a_256x240.png
        reviewboard/static/rb/css/images/ui-bg_flat_55_fbec88_40x100.png
        reviewboard/static/rb/css/images/ui-bg_gloss-wave_55_5c9ccc_500x100.png
        reviewboard/static/rb/js/views/extensionBrowserView.js
        reviewboard/static/rb/css/images/ui-icons_217bc0_256x240.png
        reviewboard/static/rb/css/images/animated-overlay.gif
        reviewboard/static/rb/css/images/ui-bg_glass_75_d0e5f5_1x400.png
    
    
  2. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  4. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 83
     W291 trailing whitespace
    
  5. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (82 > 79 characters)
    
  7. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  8. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  10. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  11. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  12. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  13. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  14. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 61
     W291 trailing whitespace
    
  15. reviewboard/extensionbrowser/base.py (Diff revision 5)
     
     
    Show all issues
    Col: 53
     E127 continuation line over-indented for visual indent
    
  16. reviewboard/extensionbrowser/urls.py (Diff revision 5)
     
     
    Show all issues
    Col: 5
     E128 continuation line under-indented for visual indent
    
  17. reviewboard/webapi/resources.py (Diff revision 5)
     
     
    Show all issues
    Col: 13
     E123 closing bracket does not match indentation of opening bracket's line
    
  18. 
      
AY
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
        reviewboard/webapi/resources.py
        reviewboard/extensionbrowser/views.py
        reviewboard/__init__.py
        reviewboard/extensionbrowser/forms.py
        reviewboard/extensionbrowser/base.py
        reviewboard/urls.py
        reviewboard/extensionbrowser/urls.py
      Ignored Files:
        reviewboard/static/rb/css/images/ui-icons_2e83ff_256x240.png
        reviewboard/static/rb/css/images/ui-bg_glass_95_fef1ec_1x400.png
        reviewboard/static/rb/css/images/ui-icons_469bdd_256x240.png
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_fcfdfd_1x100.png
        reviewboard/static/rb/css/images/ui-icons_f9bd01_256x240.png
        reviewboard/static/rb/css/extensionbrowser.less
        reviewboard/static/rb/css/images/ui-icons_d8e7f3_256x240.png
        reviewboard/static/rb/css/common.less
        reviewboard/extensionbrowser/templates/extensionbrowser/extension_browser.html
        reviewboard/static/rb/js/models/extensionInformationModel.js
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_f5f8f9_1x100.png
        reviewboard/static/rb/js/extensionbrowser.js
        reviewboard/static/rb/css/jquery-ui-1.7.1.custom.css
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/images/ui-bg_flat_0_aaaaaa_40x100.png
        reviewboard/static/rb/css/images/ui-bg_glass_85_dfeffc_1x400.png
        reviewboard/static/rb/css/images/ui-icons_6da8d5_256x240.png
        reviewboard/static/rb/css/images/ui-icons_cd0a0a_256x240.png
        reviewboard/static/rb/css/images/ui-bg_flat_55_fbec88_40x100.png
        reviewboard/static/rb/css/images/ui-bg_gloss-wave_55_5c9ccc_500x100.png
        reviewboard/static/rb/js/views/extensionBrowserView.js
        reviewboard/static/rb/css/images/ui-icons_217bc0_256x240.png
        reviewboard/static/rb/css/images/animated-overlay.gif
        reviewboard/static/rb/css/images/ui-bg_glass_75_d0e5f5_1x400.png
    
    
  2. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    Col: 53
     E127 continuation line over-indented for visual indent
    
  10. reviewboard/extensionbrowser/urls.py (Diff revision 6)
     
     
    Show all issues
    Col: 5
     E128 continuation line under-indented for visual indent
    
  11. reviewboard/webapi/resources.py (Diff revision 6)
     
     
    Show all issues
    Col: 13
     E123 closing bracket does not match indentation of opening bracket's line
    
  12. 
      
mike_conley
  1. This is looking great!
    
    First off, r.rb.org is misbehaving, and I seem to be unable to delete comments - thus, I've edited them to say [comment deleted]. Ech.
    
    I've refrained from poking too much at the CSS and interface stuff, since, as you mention, it'll likely change a lot over the next little while, and we just want to get it in a testable state while we develop the store.
    
    Basically, I think this looks really good! I have one concern about the JSONDecodeError (see below), and a few general nits. But otherwise, I like the look of this.
    
    -Mike
    
  2. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    [comment deleted]
  3. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    [comment deleted]
  4. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    On attempting a search with this latest patch, I'm getting this trace:
    
    http://pastie.org/7308158
    
    Apparently, json.JSONDecodeError doesn't exist for my version of the Django simplejson library (I'm on Django 1.4.5).
    
    If this exception is a recent addition to the library, we should check to see if we need to bump our Django requirements. Alternatively, we could throw a ValueError exception here.
    1. That's weird, I'm on 1.4.5 as well and it seems to work there. I've changed it to catch ValueError instead; tested it with a malformed JSON and it works.
  5. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
    Show all issues
    [comment deleted]
  6. reviewboard/extensionbrowser/base.py (Diff revision 6)
     
     
     
    Show all issues
    [comment deleted]
  7. Show all issues
    Trailing whitespace.
  8. Show all issues
    Trailing whitespace
  9. Show all issues
    Trailing whitespace
  10. Show all issues
    var TabbedModalBoxView
  11. Show all issues
    typo: install -> installs
  12. Show all issues
    var InfoView
  13. Show all issues
    Trailing whitespace
  14. Show all issues
    Nit - indent one more space.
  15. reviewboard/webapi/resources.py (Diff revision 6)
     
     
     
    Show all issues
    Swap INVALID_FORM_DATA and INSTALL_EXTENSION_FAILED.
  16. 
      
AY
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/settings.py
        reviewboard/webapi/resources.py
        reviewboard/extensionbrowser/views.py
        reviewboard/__init__.py
        reviewboard/extensionbrowser/forms.py
        reviewboard/extensionbrowser/base.py
        reviewboard/urls.py
        reviewboard/extensionbrowser/urls.py
      Ignored Files:
        reviewboard/static/rb/css/images/ui-icons_2e83ff_256x240.png
        reviewboard/static/rb/css/images/ui-bg_glass_95_fef1ec_1x400.png
        reviewboard/static/rb/css/images/ui-icons_469bdd_256x240.png
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_fcfdfd_1x100.png
        reviewboard/static/rb/css/images/ui-icons_f9bd01_256x240.png
        reviewboard/static/rb/css/extensionbrowser.less
        reviewboard/static/rb/css/images/ui-icons_d8e7f3_256x240.png
        reviewboard/static/rb/css/common.less
        reviewboard/extensionbrowser/templates/extensionbrowser/extension_browser.html
        reviewboard/static/rb/js/models/extensionInformationModel.js
        reviewboard/static/rb/css/images/ui-bg_inset-hard_100_f5f8f9_1x100.png
        reviewboard/static/rb/js/extensionbrowser.js
        reviewboard/static/rb/css/jquery-ui-1.7.1.custom.css
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/images/ui-bg_flat_0_aaaaaa_40x100.png
        reviewboard/static/rb/css/images/ui-bg_glass_85_dfeffc_1x400.png
        reviewboard/static/rb/css/images/ui-icons_6da8d5_256x240.png
        reviewboard/static/rb/css/images/ui-icons_cd0a0a_256x240.png
        reviewboard/static/rb/css/images/ui-bg_flat_55_fbec88_40x100.png
        reviewboard/static/rb/css/images/ui-bg_gloss-wave_55_5c9ccc_500x100.png
        reviewboard/static/rb/js/views/extensionBrowserView.js
        reviewboard/static/rb/css/images/ui-icons_217bc0_256x240.png
        reviewboard/static/rb/css/images/animated-overlay.gif
        reviewboard/static/rb/css/images/ui-bg_glass_75_d0e5f5_1x400.png
    
    
  2. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  8. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Col: 45
     E128 continuation line under-indented for visual indent
    
  9. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Col: 53
     E127 continuation line over-indented for visual indent
    
  10. reviewboard/extensionbrowser/urls.py (Diff revision 7)
     
     
    Show all issues
    Col: 5
     E128 continuation line under-indented for visual indent
    
  11. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Show all issues
    Col: 13
     E123 closing bracket does not match indentation of opening bracket's line
    
  12. 
      
chipx86
  1. 
      
  2. reviewboard/__init__.py (Diff revision 7)
     
     
     
     
     
     
     
    Show all issues
    Can we pull this out into its own patch? I'd like to get it into a release.
    
    Also, I think you can just use settings.LOCAL_ROOT instead of figuring it out from settings_local.
  3. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
     
     
    Show all issues
    No blank line here.
  4. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    We should rename 'id' to something else. id is a reserved keyword.
  5. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
     
     
     
     
     
    Show all issues
    These blank lines should be removed.
  6. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Blank line before this.
  7. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
     
     
     
     
     
     
    Show all issues
    There are alignment problems here.
  8. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Comma after "dependencies"
  9. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Blank line before.
  10. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
    Show all issues
    Generally we'll do:
    
    return {
        'error': ...,
    }
  11. reviewboard/extensionbrowser/base.py (Diff revision 7)
     
     
     
    Show all issues
    Kind of awkwardly formatted. I'd prefer:
    
    response[1][....] = \
        package_name in self.installed_extension
  12. Show all issues
    Space before the "/>"
  13. Show all issues
    IDs like "results" make me nervous. Far too generic. Prefixing a lot of these IDs with something to make them pretty unique would be good.
  14. Show all issues
    These should be indented relative to other template tags. They're too far indented right now. Same with the other nearby tags.
  15. Show all issues
    Missing {% trans %} tags.
  16. Show all issues
    You shouldn't need the "./" part.
  17. Show all issues
    Blank line before this.
  18. Show all issues
    Indent "compressed_js" one space.
  19. reviewboard/extensionbrowser/views.py (Diff revision 7)
     
     
    Show all issues
    Blank line before this.
  20. reviewboard/extensionbrowser/views.py (Diff revision 7)
     
     
    Show all issues
    Blank line before this.
  21. reviewboard/extensionbrowser/views.py (Diff revision 7)
     
     
     
     
    Show all issues
    No blank line here.
  22. reviewboard/extensionbrowser/views.py (Diff revision 7)
     
     
     
    Show all issues
    Should be formatted like:
    
    params = {
        'query': ...,
        'offset': ...,
    }
  23. reviewboard/extensionbrowser/views.py (Diff revision 7)
     
     
    Show all issues
    Blank line before this.
  24. reviewboard/extensionbrowser/views.py (Diff revision 7)
     
     
    Show all issues
    Comma after.
  25. reviewboard/settings.py (Diff revision 7)
     
     
     
    Show all issues
    Swap these. Alphabetical order.
  26. reviewboard/static/rb/css/extensionbrowser.less (Diff revision 7)
     
     
     
     
     
     
     
     
    Show all issues
    Should be nesting these better in the li above, like:
    
    li {
        ...
    
        &.header div {
            ...
        }
    
        div {
            ...
        }
    }
  27. reviewboard/static/rb/css/extensionbrowser.less (Diff revision 7)
     
     
     
     
    Show all issues
    Can this be removed now?
  28. reviewboard/static/rb/js/extensionbrowser.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    The name should be a bit more specific, name-wise.
    
    I'd also feel better with this file not being here in the root (we're trying to empty out the js/ directory and keep things in subdirs). I'd prefer all this code be on a view that is instantiated inside the HTML of a page.
  29. Show all issues
    Defaults goes first.
  30. Show all issues
    This is an unsafe assumption.
    
    You should use:
    
        SITE_ROOT + "api/..."
  31. Show all issues
    No blank line here.
  32. Show all issues
    Only one model per file. This needs to be in a separate file.
  33. Show all issues
    defaults goes first.
  34. Show all issues
    Same comment as above.
  35. Show all issues
    Why's this blank?
  36. Show all issues
    No blank line.
  37. Show all issues
    Remove this. We get it by default.
  38. Show all issues
    I'm not sure we need this. I'd prefer not using _bindAll unless we have specific uses. In general, we should be calling render() with the right context.
  39. Show all issues
    As in my previous comment, this should be using _.template, and joining an array of lines.
  40. Show all issues
    One var statement per function, at the top of the function, with each parameter being on its own line, comma-separated.
    
    For the _.template, this should be created on the class itself.
  41. Show all issues
    Should be:
    
        this.$el
            .html(template)
            .modalBox();
    
    As for the class, that should go in the className field at the top of the class.
  42. Show all issues
    Should be:
    
        new InfoView({
            model: this.model
        })
  43. Show all issues
    No blank line.
  44. Show all issues
    Each template should be compiled here, once, and not later.
  45. Show all issues
    Remove this. We get it by default.
  46. Show all issues
    Same comment as above.
  47. Show all issues
    render shouldn't take an ID.
    
    I think if we have this one thing representing different tabs, we should really just have each tab subclass this and provide their own template/events.
  48. Show all issues
    Should be done inline above, so it's only ever done once per page.
  49. Show all issues
    this.$el
  50. Show all issues
    A view should never pack itself. That's up to the code creating and rendering the class.
  51. Show all issues
    The dictionary fields should be indented 1 level from the "var extension_install".
    
    Also, extension_install should be extensionInstallInfo. (No underscores in variables in the JavaScript code anywhere.)
  52. Show all issues
    No need for the 'self'. Instead, pass 'this' as the second parameter to save(), which will bind the proper context.
  53. reviewboard/webapi/resources.py (Diff revision 7)
     
     
     
    Show all issues
    Alphabetical order.
  54. reviewboard/webapi/resources.py (Diff revision 7)
     
     
     
     
    Show all issues
    No blank line.
  55. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Show all issues
    This is too generic, given the context.
    
    This file's too big. I'd recommend we start breaking this out. Maybe put this into extension_store.py.
  56. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Show all issues
    Summaries can only be one line.
  57. reviewboard/webapi/resources.py (Diff revision 7)
     
     
     
    Show all issues
    There should be a has_access_permissions function that does this, and the get function should call that.
  58. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Show all issues
    Put package_name in the parameter list for the function, and then pass to get_links below.
  59. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Show all issues
    Blank line before this.
  60. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Show all issues
    Not enough context in the log statement.
  61. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Show all issues
    Should be removed.
  62. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Show all issues
    The fields should be on their own lines, just like other resources.
  63. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Show all issues
    This shouldn't be able to work. It will turn into ('P', 'O', 'S', 'T'). Make sure it's ('POST',)
  64. reviewboard/webapi/resources.py (Diff revision 7)
     
     
     
    Show all issues
    Same comment as above.
  65. reviewboard/webapi/resources.py (Diff revision 7)
     
     
     
     
     
    Show all issues
    Alignment issue.
    
    The _entrypoint_iterator() is private and shouldn't be used. There'd need to be an accessor.
  66. reviewboard/webapi/resources.py (Diff revision 7)
     
     
     
    Show all issues
    The wrapping is weird.
    
    You're calling get_extension_manager() a lot. Call it once and just use that variable.
  67. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    Show all issues
    There needs to be more context.
    
    I think, though, that install_extension should be logging.
  68. reviewboard/webapi/resources.py (Diff revision 7)
     
     
     
     
    Show all issues
    Summaries can only be one line.
  69. 
      
david
Review request changed
Status:
Discarded