User is able to specify their own hub in extension configurations.

Review Request #6102 — Created July 13, 2014 and submitted

Information

rb-extension-pack
9ff944c...

Reviewers

User is able to specify their own hub in extension configurations.

A hub server was run locally. Its URL was used in the extension configuration. The local server was used successfully to support the extension.

To test the hub used is actually the one indicated:
Launch the chat app while the local server is not on. The chat should still launch but using the "add friend" link on another browser should not connect the two clients.

To test the default hub is used when no hub is specified, remove any input in the extension configuration screen. Launch the chat app and using another browser connect to the first client with the "Add friend" link. The clients should be able to see each other's mouse movements.

Description From Last Updated

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

This isn't used anymore (since 'urlpatterns' is redefined below).

daviddavid

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

reviewbotreviewbot

Col: 2 W292 no newline at end of file

reviewbotreviewbot

'settings' imported but unused

reviewbotreviewbot

'patterns' imported but unused

reviewbotreviewbot

'include' imported but unused

reviewbotreviewbot

'HeaderDropdownActionHook' imported but unused

reviewbotreviewbot

This should probably be "TogetherJS", and wrapped in _()

daviddavid

This doesn't seem to be used.

daviddavid

None of this is used (you're using the built-in configure view now)

daviddavid

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 5 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

All of this could go on the same line.

daviddavid

I'm thinking we should probably bundle the togetherjs script and include it in the js bundle. Then we can get …

daviddavid

This doesn't seem like a very good label.

daviddavid

This should probably be kept inside the TogetherJS.Extension class.

daviddavid

Should be 4-space indent.

daviddavid

Doesn't this shadow the TogetherJS function?

daviddavid

Is this variable used inside TogetherJS's code? Does it really need to be re-defined here?

daviddavid

Variable definitions should be the first line in the function. The superclass method should also be called with any arguments: …

daviddavid

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 9 E122 continuation line missing indentation or outdented

reviewbotreviewbot

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

reviewbotreviewbot

This should be: urlpatterns = patterns( '', url(r'^$', 'reviewboard.....', { ... }) ) That will fix your ReviewBot problem. When …

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

This, and lines 4-8 seem redundant after what's going on in initialize.

mike_conleymike_conley

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

reviewbotreviewbot

Col: 5 E124 closing bracket does not match visual indentation

reviewbotreviewbot

This can be deleted now, right?

daviddavid

Add a trailing comma here.

daviddavid

'TemplateHook' imported but unused

reviewbotreviewbot

We shouldn't make this a requirement for people to get up and running. We should make it default to the …

mike_conleymike_conley

"Copy or write down" -> "Make note of".

mike_conleymike_conley

Trailing whitespace.

daviddavid

This file was in a different change. Can you pull the latest master and rebase your branch?

daviddavid

Instead of defining these up here, just define them when they're first assigned.

mike_conleymike_conley

What happens if hub_url is not defined? The reason I ask, is that we might want to make it easy …

mike_conleymike_conley

This file shouldn't be called "*-min.js" because it's not minified.

daviddavid

Nit - instead of "where TogetherJS is listed", it'd be "where ReviewTogether is listed", or whatever is actually listed in …

mike_conleymike_conley

You might want to double-check with one of the other mentors that this style of breaking lines is legit. Looks …

mike_conleymike_conley

reviewboard -> Review Board

mike_conleymike_conley

let's break the list over two lines: 'source_filenames': ['js/togetherjs.js', 'js/review-together.js',]

mike_conleymike_conley

ReviewBoard -> Review Board.

mike_conleymike_conley

I don't think this comment adds much - can be safely removed.

mike_conleymike_conley

This change should have already landed as part of your change from /r/6066... Are you sure you have an up-to-date …

mike_conleymike_conley

Like I said in IRC, let's just do: if (settings.hub_url) { // Set the TogetherJSConfig_hubBase variable }

mike_conleymike_conley

Ok, now I'm really confused... This patch is the base of /r/6194 - I get that... But what is this …

mike_conleymike_conley

'JSExtension' imported but unused

reviewbotreviewbot

undefined name 'ReviewTogetherJSExtension'

reviewbotreviewbot
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/views.py
    
    Ignored Files:
        review_together/review_together/templates/review_together/configure.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/views.py
    
    Ignored Files:
        review_together/review_together/templates/review_together/configure.html
    
    
  2. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  3. Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/views.py
    
    Ignored Files:
        review_together/review_together/templates/review_together/configure.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/views.py
    
    Ignored Files:
        review_together/review_together/templates/review_together/configure.html
    
    
  2. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/views.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/templates/review_together/configure.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/views.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/templates/review_together/configure.html
    
    
  2. Show all issues
    Col: 5
     E128 continuation line under-indented for visual indent
    
  3. Show all issues
    Col: 2
     W292 no newline at end of file
    
  4. Show all issues
     'settings' imported but unused
    
  5. Show all issues
     'patterns' imported but unused
    
  6. Show all issues
     'include' imported but unused
    
  7. Show all issues
     'HeaderDropdownActionHook' imported but unused
    
  8. 
      
david
  1. 
      
  2. review_together/review_together/admin_urls.py (Diff revision 3)
     
     
     
     
    Show all issues

    This isn't used anymore (since 'urlpatterns' is redefined below).

  3. Show all issues

    This should probably be "TogetherJS", and wrapped in _()

    1. The label should be TogetherJS and wrapped in ()? What does () do?

    2. Not just parens, but _(), which should be imported using from django.utils.translation import ugettext_lazy as _

      It marks the string for translation.

  4. review_together/review_together/templates/review_together/configure.html (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This doesn't seem to be used.

  5. review_together/review_together/views.py (Diff revision 3)
     
     
     
     
     
     
     
    Show all issues

    None of this is used (you're using the built-in configure view now)

  6. 
      
PE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/templates/review_together/base.html
    
    
  2. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  3. Show all issues
    Col: 2
     E126 continuation line over-indented for hanging indent
    
  4. Show all issues
    Col: 1
     E101 indentation contains mixed spaces and tabs
    
  5. Show all issues
    Col: 5
     E131 continuation line unaligned for hanging indent
    
  6. 
      
PE
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/templates/review_together/base.html
    
    
  2. 
      
david
  1. 
      
  2. review_together/review_together/admin_urls.py (Diff revision 5)
     
     
     
     
    Show all issues

    All of this could go on the same line.

  3. Show all issues

    I'm thinking we should probably bundle the togetherjs script and include it in the js bundle. Then we can get rid of this template entirely.

  4. Show all issues

    This doesn't seem like a very good label.

  5. review_together/review_together/static/js/together.js (Diff revision 5)
     
     
     
     
     
     
     
    Show all issues

    This should probably be kept inside the TogetherJS.Extension class.

  6. Show all issues

    Should be 4-space indent.

  7. Show all issues

    Doesn't this shadow the TogetherJS function?

  8. Show all issues

    Is this variable used inside TogetherJS's code? Does it really need to be re-defined here?

  9. Show all issues

    Variable definitions should be the first line in the function. The superclass method should also be called with any arguments:

    initialize: function() {
        var settings;
    
        _super(this).initialize.apply(this, arguments);
    
        settings = this.get('settings');
        TogetherJSConfig_hubBase = settings.hub_url;
    }
    
  10. 
      
PE
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/templates/review_together/base.html
    
    
  2. Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. Show all issues
    Col: 9
     E128 continuation line under-indented for visual indent
    
  4. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
    
    
  2. Show all issues
    Col: 80
     E501 line too long (89 > 79 characters)
    
  3. Show all issues
    Col: 9
     E128 continuation line under-indented for visual indent
    
  4. 
      
PE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
    
    
  2. Show all issues
    Col: 2
     E128 continuation line under-indented for visual indent
    
  3. Show all issues
    Col: 1
     W191 indentation contains tabs
    
  4. Show all issues
    Col: 1
     E101 indentation contains mixed spaces and tabs
    
  5. Show all issues
    Col: 9
     E122 continuation line missing indentation or outdented
    
  6. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
    
    
  2. Show all issues
    Col: 25
     E127 continuation line over-indented for visual indent
    
  3. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
    
    
  2. Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  3. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
  2. Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  3. 
      
mike_conley
  1. 
      
  2. Show all issues

    This, and lines 4-8 seem redundant after what's going on in initialize.

  3. Is TogetherJSConfig_hubBase a global that the TogetherJS library reads?

  4. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
  2. Show all issues
    Col: 21
     E128 continuation line under-indented for visual indent
    
  3. 
      
chipx86
  1. 
      
  2. review_together/review_together/admin_urls.py (Diff revision 10)
     
     
     
     
     
     
     
     
    Show all issues

    This should be:

    urlpatterns = patterns(
        '',
    
        url(r'^$',
            'reviewboard.....',
            {
                ...
            })
    )
    

    That will fix your ReviewBot problem.

    When in doubt, look at how the rest of the Review Board codebase is doing things, for inspiration.

  3. 
      
PE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
  2. Show all issues
    Col: 5
     E124 closing bracket does not match visual indentation
    
  3. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
  2. 
      
david
  1. 
      
  2. review_together/review_together/extension.py (Diff revision 14)
     
     
     
     
    Show all issues

    This can be deleted now, right?

  3. Show all issues

    Add a trailing comma here.

  4. 
      
PE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/README.md
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/README.md
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
  2. Show all issues
     'TemplateHook' imported but unused
    
  3. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/README.md
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs-min.js
        review_together/README.md
        review_together/review_together/static/js/together.js
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
  2. 
      
david
  1. 
      
  2. review_together/README.md (Diff revision 16)
     
     
    Show all issues

    Trailing whitespace.

  3. Show all issues

    This file was in a different change. Can you pull the latest master and rebase your branch?

  4. Show all issues

    This file shouldn't be called "*-min.js" because it's not minified.

  5. 
      
mike_conley
  1. 
      
  2. review_together/README.md (Diff revision 16)
     
     
    Show all issues

    We shouldn't make this a requirement for people to get up and running. We should make it default to the Mozilla hub, but then tell them how to override with their own hub here.

  3. review_together/README.md (Diff revision 16)
     
     
    Show all issues

    "Copy or write down" -> "Make note of".

  4. Show all issues

    Instead of defining these up here, just define them when they're first assigned.

  5. Show all issues

    What happens if hub_url is not defined? The reason I ask, is that we might want to make it easy for people to get Review Together up and running and working, without making them set up a Hub server.

    So is it possible to have this default to the Mozilla hub server if blank?

  6. 
      
PE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs.js
        review_together/review_together/static/js/review-together.js
        review_together/README.md
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs.js
        review_together/review_together/static/js/review-together.js
        review_together/README.md
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
  2. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs.js
        review_together/review_together/static/js/review-together.js
        review_together/README.md
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs.js
        review_together/review_together/static/js/review-together.js
        review_together/README.md
        review_together/review_together/static/css/review-together.less
        review_together/review_together/templates/review_together/base.html
    
    
  2. 
      
mike_conley
  1. 
      
  2. review_together/README.md (Diff revision 18)
     
     
    Show all issues

    Nit - instead of "where TogetherJS is listed", it'd be "where ReviewTogether is listed", or whatever is actually listed in the Extension manager.

  3. review_together/review_together/admin_urls.py (Diff revision 18)
     
     
     
     
     
     
     
    Show all issues

    You might want to double-check with one of the other mentors that this style of breaking lines is legit. Looks like pep8 likes it, but something looks off here. Maybe I'm just imagining it though.

    1. Anselina recommended I copy: https://github.com/reviewboard/rb-extension-pack/blob/master/rbmotd/rbmotd/admin_urls.py

  4. Show all issues

    reviewboard -> Review Board

  5. Show all issues

    let's break the list over two lines:

    'source_filenames': ['js/togetherjs.js',
    'js/review-together.js',]

  6. Show all issues

    ReviewBoard -> Review Board.

  7. review_together/review_together/extension.py (Diff revision 18)
     
     
     
    Show all issues

    I don't think this comment adds much - can be safely removed.

  8. Show all issues

    This change should have already landed as part of your change from /r/6066...

    Are you sure you have an up-to-date rb-extension-pack repo?

  9. Show all issues

    Like I said in IRC, let's just do:

    if (settings.hub_url) {
    // Set the TogetherJSConfig_hubBase variable
    }

  10. 
      
PE
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs.js
        review_together/review_together/static/js/review-together.js
        review_together/README.md
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs.js
        review_together/review_together/static/js/review-together.js
        review_together/README.md
    
    
  2. 
      
mike_conley
  1. What's the empty forms.py for?

    1. Just re-posted. forms.py is nonempty and ready for review.

  2. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs.js
        review_together/review_together/static/js/review-together.js
        review_together/README.md
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/togetherjs.js
        review_together/review_together/static/js/review-together.js
        review_together/README.md
    
    
  2. 
      
mike_conley
  1. Hrm - something is fishy here... it looks like your review request is based on code that's not yet landed. For example, looking at the diff, I see you mucking about in JS files under review_together/static/js... but on master, no such directory exists: https://github.com/reviewboard/rb-extension-pack/tree/master/review_together/review_together/static

    1. Hi Mike,

      Should I rebase off master? I think I rebased on my other branch that has not been landed.

    2. Ah, if that's the case, please mark the dependency with the Depends On field in this review request. Mark this review request as Depending On the other.

  2. 
      
mike_conley
  1. 
      
  2. review_together/README.md (Diff revision 20)
     
     
    Show all issues

    Ok, now I'm really confused...

    This patch is the base of /r/6194 - I get that...

    But what is this patch based off of? The changes in this patch appear to be applied on top of things that are not yet in the rb-extensions-pack/review_together folder.

    For example, this line: "To get Review Together running, you will need to run a TogetherJS hub."

    That you're changing... where did that get introduced? Did we miss a commit somewhere?

  3. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/review-together.js
        review_together/README.md
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/review-together.js
        review_together/README.md
    
    
  2. Show all issues
     'JSExtension' imported but unused
    
  3. Show all issues
     undefined name 'ReviewTogetherJSExtension'
    
  4. 
      
PE
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/review-together.js
        review_together/README.md
    
    
    
    Tool: Pyflakes
    Processed Files:
        review_together/review_together/forms.py
        review_together/review_together/admin_urls.py
        review_together/review_together/extension.py
    
    Ignored Files:
        review_together/review_together/static/js/review-together.js
        review_together/README.md
    
    
  2. 
      
david
  1. I'm going to make some small changes to the docs and then push this.

  2. 
      
PE
Review request changed
Status:
Completed
Change Summary:
Pushed to master (982605d)