RBTools Shell command for preconfigured Python shell environment

Review Request #7936 — Created Jan. 31, 2016 and submitted

aquafemi
RBTools
master
c73fa56...
rbtools, students

This new command rbt shell opens a Python shell. Works with Ipython and bpython. Imports configuration files and command options into that shell as dictionaries.

Makes a client connection to a server if one is provided either as a command option or configuration option.

Can work with Ipython and bpython if installed.

Also added the command to rbtools' setup.py.

Also added documentation to rbtools/docs/rbtools/rbt/commands/shell.rst

Created test cases for shell rbtools/rbtools/commands/tests.py

Manual Testing:

Ran the command to see if it works.

Automatically made a client connection by typing rbt shell, and also with providing a server url.

Automatically made a client connection by providing configuration keys.

Successfully made api calls from within the shell.

Installed Ipython and bpython with pip and made sure that both opened.

Tested to see that shell would still open if an invalid shell was specified.

Automated Testing:

Test to see if Ipython module import is called

Test to see if bpython module import is called

Test messages being logged to the shell

Description From Last Updated

Col: 6 E113 unexpected indentation

reviewbotreviewbot

Col: 6 E111 indentation is not a multiple of four

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

This looks good as a first draft You may want to have a look at https://github.com/django/django/blob/master/django/core/management/commands/shell.py Also, I'd personally love ...

brenniebrennie

Blank line between these.

brenniebrennie

'logging' imported but unused

reviewbotreviewbot

'Option' imported but unused

reviewbotreviewbot

'LogLevelFilter' imported but unused

reviewbotreviewbot

'OptionGroup' imported but unused

reviewbotreviewbot

'get_config_paths' imported but unused

reviewbotreviewbot

'load_config' imported but unused

reviewbotreviewbot

'parse_config_file' imported but unused

reviewbotreviewbot

'execute' imported but unused

reviewbotreviewbot

'die' imported but unused

reviewbotreviewbot

New commands should not use die(). They should raise an exception.

brenniebrennie

This should be written in the imperitive mood. i.e., 'Start a ...'

brenniebrennie

How about: 'This command will inject variables into a new interactive shell from your .reviewboardrc.`

brenniebrennie

You probably don't want to copy wholesale from django's shell for the final version.

brenniebrennie

Col: 32 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 34 E231 missing whitespace after ','

reviewbotreviewbot

Col: 38 E231 missing whitespace after ':'

reviewbotreviewbot

Single quotes for strings.

brenniebrennie

Why not just have readline and rlcompleter be dependencies (in setup.py) so that we're guaranteed they exist?

brenniebrennie

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

reviewbotreviewbot

Col: 30 E225 missing whitespace around operator

reviewbotreviewbot

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

reviewbotreviewbot

Col: 34 E711 comparison to None should be 'if cond is not None:'

reviewbotreviewbot

Col: 36 E711 comparison to None should be 'if cond is not None:'

reviewbotreviewbot

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

reviewbotreviewbot

What, exactly, are these config files going to be?

brenniebrennie

Should use sentence casing. Needs a period.

brenniebrennie

Parens not necessary here.

brenniebrennie

Parens not necessary here.

brenniebrennie

'RBClient' imported but unused

reviewbotreviewbot

'CONFIG_FILE' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'e' is assigned to but never used

reviewbotreviewbot

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

reviewbotreviewbot

I don't think we need to include this, since the attribute is being added at the same time as the ...

daviddavid

No blank line necessary here.

daviddavid

This is a little cleaner as: except Exception as e: logging.error('Could not run script "%s" for RBTools shell: %s", script_path, ...

daviddavid

You can skip the conditional if you do this instead; for import_command in config.get('SHELL_IMPORTS', []):

daviddavid

A better way to wrap this is to put the stuff in the parens on the next line: readline.set_completer( rlcompleter.Completer(namespace).complete) ...

daviddavid

'logging' imported but unused

reviewbotreviewbot

'os' imported but unused

reviewbotreviewbot

'sys' imported but unused

reviewbotreviewbot

'Option' imported but unused

reviewbotreviewbot

'load_config' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 9 E124 closing bracket does not match visual indentation

reviewbotreviewbot

Col: 9 E124 closing bracket does not match visual indentation

reviewbotreviewbot

I think it would be useful to have a config key that allows you to use ipython or bpython. e.g., ...

brenniebrennie

"pre-configured"

brenniebrennie

I would have these both use the same config key and use store_const e.g. option_list = [ Option('-i', '--ipython', config_key='SHELL', ...

brenniebrennie

I feel like we should have namespace[config] = self.config namespace[options] = vars(self.options) so that we don't pollute namespace.

brenniebrennie

Not necessary.

brenniebrennie

Not necessary.

brenniebrennie

We should inform the user that, if we can't find their server settings, that client and root are not defined. ...

brenniebrennie

Not necessary.

brenniebrennie

You can move the code.interact() call to below the except: block if you return early in the iPython or bpython ...

brenniebrennie

Doesn't need parentheses.

brenniebrennie

Use a dict literal: namespace = { 'config': self.config, 'options': vars(self.options), }

brenniebrennie

Can we add a --quiet/ -q option (that can be controlled by a .reviewboardrc option too) to disable these statements?

brenniebrennie

This should use logging.exception and print an error message about what failed.

brenniebrennie

Same comment about -q.

brenniebrennie

This should probably be: elif shell Otherwise if shell is None (for default Python shell), it will always print this.

brenniebrennie

I dont think we need this.

brenniebrennie

Python

brenniebrennie

Suppress

brenniebrennie

Docstring w/ args?

brenniebrennie

Blank line between these.

brenniebrennie

Blank line between these.

brenniebrennie

'spy_on' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Your test case should inheirit from SpyAgency. Then in unit tests you can use self.spy_on(foo, ...) without having to set ...

brenniebrennie

Not too sure what this line is doing. Did the test intend to call shell.ipython() instead of assigning the classmethod ...

SS ssengar

local variable 'ipython_module' is assigned to but never used

reviewbotreviewbot

If the objective of the test was to make sure shell.bpython wasn't called, it should probably spy on it explicitly ...

SS ssengar

Again, did the test intend to call shell.bpython() instead of assigning the classmethod to a variable?

SS ssengar

local variable 'bpython_module' is assigned to but never used

reviewbotreviewbot

If you add SpyAgency as a parent class, you can use self.spy_on(shell.method_name). This has several advantages: you don't have to ...

SS ssengar

local variable 'bpython_module' is assigned to but never used

reviewbotreviewbot

Maybe I'm missing something but is there a need to spy on shell.ipython at the end of the test? If ...

SS ssengar

local variable 'ipython_module' is assigned to but never used

reviewbotreviewbot

Same as comment for line 61.

SS ssengar

local variable 'client' is assigned to but never used

reviewbotreviewbot

Again, is the spy operation on shell.bpython repated for a particular reason?

SS ssengar

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

'spy_on' imported but unused

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

Col: 34 W292 no newline at end of file

reviewbotreviewbot

Can you break these up into multiple test cases, e.g.: def test_ipython(self): # raise SkipTest if we don't have ipython ...

brenniebrennie

These tests aren't actually testing anything.

brenniebrennie

shell.ipython can never return None. maybe update the method to return none if it cant get the shell?

brenniebrennie

These tests dont test the results. See above comments.

brenniebrennie

These don't test the results of calling log_message.

brenniebrennie

Needs "Returns"

brenniebrennie

Needs "returns"

brenniebrennie

Blank line between these.

brenniebrennie

No periods at the end of test cases. Same for all test cases in this file.

brenniebrennie

You don't need unspy calls at the end of test -- they will automatically be called.

brenniebrennie

Can you split this into multiple tests?

brenniebrennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. rbtools/commands/shell.py (Diff revision 1)
     
     
    Col: 6
     E113 unexpected indentation
    
  3. rbtools/commands/shell.py (Diff revision 1)
     
     
    Col: 6
     E111 indentation is not a multiple of four
    
  4. rbtools/commands/shell.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. rbtools/commands/shell.py (Diff revision 1)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  6. 
      
AQ
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/shell.py (Diff revision 2)
     
     

    This looks good as a first draft

    You may want to have a look at https://github.com/django/django/blob/master/django/core/management/commands/shell.py

    Also, I'd personally love it if this supported iPython and regular Python (via a flag / config option)

    1. Oops, put this comment in the wrong place :P I had intended this not to be an issue

  3. rbtools/commands/shell.py (Diff revision 2)
     
     
     

    Blank line between these.

  4. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. rbtools/commands/shell.py (Diff revision 3)
     
     
     'logging' imported but unused
    
  3. rbtools/commands/shell.py (Diff revision 3)
     
     
     'Option' imported but unused
    
  4. rbtools/commands/shell.py (Diff revision 3)
     
     
     'LogLevelFilter' imported but unused
    
  5. rbtools/commands/shell.py (Diff revision 3)
     
     
     'OptionGroup' imported but unused
    
  6. rbtools/commands/shell.py (Diff revision 3)
     
     
     'get_config_paths' imported but unused
    
  7. rbtools/commands/shell.py (Diff revision 3)
     
     
     'load_config' imported but unused
    
  8. rbtools/commands/shell.py (Diff revision 3)
     
     
     'parse_config_file' imported but unused
    
  9. rbtools/commands/shell.py (Diff revision 3)
     
     
     'execute' imported but unused
    
  10. rbtools/commands/shell.py (Diff revision 3)
     
     
     'die' imported but unused
    
  11. rbtools/commands/shell.py (Diff revision 3)
     
     
    Col: 32
     E231 missing whitespace after ':'
    
  12. rbtools/commands/shell.py (Diff revision 3)
     
     
    Col: 34
     E231 missing whitespace after ','
    
  13. rbtools/commands/shell.py (Diff revision 3)
     
     
    Col: 38
     E231 missing whitespace after ':'
    
  14. rbtools/commands/shell.py (Diff revision 3)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  15. 
      
brennie
  1. 
      
  2. rbtools/commands/shell.py (Diff revision 3)
     
     

    New commands should not use die(). They should raise an exception.

  3. rbtools/commands/shell.py (Diff revision 3)
     
     

    This should be written in the imperitive mood.

    i.e., 'Start a ...'

  4. rbtools/commands/shell.py (Diff revision 3)
     
     

    How about:

    'This command will inject variables into a new interactive shell from your .reviewboardrc.`

    1. I'm still have some uncertainty about what the class will be in the end but for now I've reworded the description to "This will inject predefined variables from your .reviewboardrc into the scope of a new interactive Python shell."

  5. rbtools/commands/shell.py (Diff revision 3)
     
     

    You probably don't want to copy wholesale from django's shell for the final version.

  6. rbtools/commands/shell.py (Diff revision 3)
     
     

    Single quotes for strings.

  7. rbtools/commands/shell.py (Diff revision 3)
     
     

    Why not just have readline and rlcompleter be dependencies (in setup.py) so that we're guaranteed they exist?

    1. So digging around, here's what I've found.

      readline and rlcompleter are part of Python. The reason it's sometimes not around is that some environments don't include this support for licensing reasons. Mac didn't used to, for instance.

      I don't think this has been a real problem in a while, and Django's code for this is almost a decade old. I would just assume the presence of these libraries for now. We can address this down the road if needed.

    2. Then we should just import them as normal.

  8. 
      
AQ
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. rbtools/commands/shell.py (Diff revision 4)
     
     
    Col: 30
     E225 missing whitespace around operator
    
  3. rbtools/commands/shell.py (Diff revision 4)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. rbtools/commands/shell.py (Diff revision 4)
     
     
    Col: 34
     E711 comparison to None should be 'if cond is not None:'
    
  5. rbtools/commands/shell.py (Diff revision 4)
     
     
    Col: 36
     E711 comparison to None should be 'if cond is not None:'
    
  6. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. rbtools/commands/shell.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. 
      
AQ
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/shell.py (Diff revision 6)
     
     
     
     
     
     

    What, exactly, are these config files going to be?

    1. Removed them and now just using load_config from rbtools util.

  3. rbtools/commands/shell.py (Diff revision 6)
     
     
     

    Should use sentence casing. Needs a period.

  4. rbtools/commands/shell.py (Diff revision 6)
     
     

    Parens not necessary here.

  5. rbtools/commands/shell.py (Diff revision 6)
     
     

    Parens not necessary here.

  6. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. rbtools/commands/shell.py (Diff revision 7)
     
     
     'RBClient' imported but unused
    
  3. rbtools/commands/shell.py (Diff revision 7)
     
     
     'CONFIG_FILE' imported but unused
    
  4. rbtools/commands/shell.py (Diff revision 7)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. rbtools/commands/shell.py (Diff revision 7)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  6. rbtools/commands/shell.py (Diff revision 7)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  7. rbtools/commands/shell.py (Diff revision 7)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  8. rbtools/commands/shell.py (Diff revision 7)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  9. rbtools/commands/shell.py (Diff revision 7)
     
     
     local variable 'e' is assigned to but never used
    
  10. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. rbtools/commands/shell.py (Diff revision 8)
     
     
    Col: 18
     E127 continuation line over-indented for visual indent
    
  3. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. 
      
david
  1. This is looking pretty great!

  2. rbtools/commands/shell.py (Diff revision 9)
     
     

    I don't think we need to include this, since the attribute is being added at the same time as the command as a whole. These are there so we can document when new attributes were added to existing commands.

    We probably should have a top-level "added_in" for the command, that will generate the appropriate note in the documentation.

  3. rbtools/commands/shell.py (Diff revision 9)
     
     

    No blank line necessary here.

  4. rbtools/commands/shell.py (Diff revision 9)
     
     
     
     

    This is a little cleaner as:

    except Exception as e:
        logging.error('Could not run script "%s" for RBTools shell: %s",
                      script_path, e)
    
  5. rbtools/commands/shell.py (Diff revision 9)
     
     
     
     

    You can skip the conditional if you do this instead;

    for import_command in config.get('SHELL_IMPORTS', []):
    
  6. rbtools/commands/shell.py (Diff revision 9)
     
     
     

    A better way to wrap this is to put the stuff in the parens on the next line:

    readline.set_completer(
        rlcompleter.Completer(namespace).complete)
    

    Alternatively, you could do from rlcompleter import Completer at the top and then just use Completer(...) here.

  7. 
      
AQ
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. rbtools/commands/shell.py (Diff revision 10)
     
     
     'logging' imported but unused
    
  3. rbtools/commands/shell.py (Diff revision 10)
     
     
     'os' imported but unused
    
  4. rbtools/commands/shell.py (Diff revision 10)
     
     
     'sys' imported but unused
    
  5. rbtools/commands/shell.py (Diff revision 10)
     
     
     'Option' imported but unused
    
  6. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. rbtools/commands/shell.py (Diff revision 11)
     
     
     'load_config' imported but unused
    
  3. rbtools/commands/shell.py (Diff revision 11)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. rbtools/commands/shell.py (Diff revision 11)
     
     
    Col: 9
     E124 closing bracket does not match visual indentation
    
  5. rbtools/commands/shell.py (Diff revision 11)
     
     
    Col: 9
     E124 closing bracket does not match visual indentation
    
  6. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. 
      
AQ
brennie
  1. Hey, this change looks good. I've got some suggestions for you.

    Also, this command will need documentation. Have a look in docs/rbtools/rbt/commands/ for examples.

  2. rbtools/commands/shell.py (Diff revision 12)
     
     

    I think it would be useful to have a config key that allows you to use ipython or bpython. e.g., you could set the following in your .reviewboardrc:

    SHELL = 'ipython'
    

    or

    SHELL = 'bpython'
    

    Then you can have -i and -b override this setting.

  3. rbtools/commands/shell.py (Diff revision 12)
     
     

    "pre-configured"

  4. rbtools/commands/shell.py (Diff revision 12)
     
     
     
     
     
     
     
     
     
     
     

    I would have these both use the same config key and use store_const

    e.g.

    option_list = [
        Option('-i', '--ipython',
               config_key='SHELL',
               dest='shell',
               action='store_const',
               const='ipython', ...),
        Option('-i', '--bpython',
               dest='shell',
               action='store_const',
               const='bpython', ...)
    ]
    

    NB: This also takes into account the above suggestion for SHELL, which only has to be on one option I believe.

  5. rbtools/commands/shell.py (Diff revision 12)
     
     
     
     

    I feel like we should have

    namespace[config] = self.config
    namespace[options] = vars(self.options)
    

    so that we don't pollute namespace.

    1. Great idea. I was worried about possible issues with defining a whole bunch of variables and not telling the user that I did that. This is a good solution to that.

  6. rbtools/commands/shell.py (Diff revision 12)
     
     

    Not necessary.

  7. rbtools/commands/shell.py (Diff revision 12)
     
     

    Not necessary.

  8. rbtools/commands/shell.py (Diff revision 12)
     
     
     
     
     

    We should inform the user that, if we can't find their server settings, that client and root are not defined. (Or inform them of the opposite when they are defined. Either one is fine.)

  9. rbtools/commands/shell.py (Diff revision 12)
     
     

    Not necessary.

  10. rbtools/commands/shell.py (Diff revision 12)
     
     
     
     
     
     
     

    You can move the code.interact() call to below the except: block if you return early in the iPython or bpython cases.

    1. If I move it under the except: block, won't it always open another python shell after ipython or bpython successfully run?

    2. Not if you return above

  11. 
      
AQ
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    
  2. 
      
AQ
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. 
      
AQ
brennie
  1. 
      
  2. rbtools/commands/shell.py (Diff revision 15)
     
     
     

    Doesn't need parentheses.

  3. rbtools/commands/shell.py (Diff revision 15)
     
     
     
     

    Use a dict literal:

    namespace = {
        'config': self.config,
        'options': vars(self.options),
    }
    
    1. Alright I made this change but any particular reason?

  4. rbtools/commands/shell.py (Diff revision 15)
     
     
     
     

    Can we add a --quiet/ -q option (that can be controlled by a .reviewboardrc option too) to disable these statements?

  5. rbtools/commands/shell.py (Diff revision 15)
     
     

    This should use logging.exception and print an error message about what failed.

  6. rbtools/commands/shell.py (Diff revision 15)
     
     
     

    Same comment about -q.

  7. rbtools/commands/shell.py (Diff revision 15)
     
     

    This should probably be:

    elif shell
    

    Otherwise if shell is None (for default Python shell), it will always print this.

  8. rbtools/commands/shell.py (Diff revision 15)
     
     

    I dont think we need this.

  9. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. 
      
AQ
brennie
  1. In your description and testing done, it should be "Python" and "IPython".

  2. docs/rbtools/rbt/commands/shell.rst (Diff revision 16)
     
     
  3. rbtools/commands/shell.py (Diff revision 16)
     
     

    Suppress

  4. rbtools/commands/shell.py (Diff revision 16)
     
     

    Docstring w/ args?

  5. rbtools/commands/shell.py (Diff revision 16)
     
     
     

    Blank line between these.

  6. rbtools/commands/shell.py (Diff revision 16)
     
     
     

    Blank line between these.

  7. 
      
AQ
AQ
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. 
      
brennie
  1. Ship It!
  2. 
      
AQ
AQ
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. rbtools/commands/tests.py (Diff revision 18)
     
     
     'spy_on' imported but unused
    
  3. rbtools/commands/tests.py (Diff revision 18)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  4. rbtools/commands/tests.py (Diff revision 18)
     
     
     local variable 'ipython_module' is assigned to but never used
    
  5. rbtools/commands/tests.py (Diff revision 18)
     
     
     local variable 'bpython_module' is assigned to but never used
    
  6. rbtools/commands/tests.py (Diff revision 18)
     
     
     local variable 'bpython_module' is assigned to but never used
    
  7. rbtools/commands/tests.py (Diff revision 18)
     
     
     local variable 'ipython_module' is assigned to but never used
    
  8. rbtools/commands/tests.py (Diff revision 18)
     
     
     local variable 'client' is assigned to but never used
    
  9. 
      
SS
  1. Great work, your project seems awesome! In this review, I gave you some feedback on your test cases. I apologize in advance if some of my feedback is irrelevant/incorrect. I'm not very familiar with the rbt shell command, so most of my feedback was based on intuition + my limited knowledge of kgb spies.

  2. rbtools/commands/shell.py (Diff revision 18)
     
     
    It's really cool that your shell has so many configuration options!
  3. rbtools/commands/shell.py (Diff revision 18)
     
     
     
     
     
     
     
     
     
     
     
     
     
    It's super cool that you could smartly re-use existing shell implementations for your project!
  4. rbtools/commands/tests.py (Diff revision 18)
     
     

    Not too sure what this line is doing. Did the test intend to call shell.ipython() instead of assigning the classmethod to a variable?

  5. rbtools/commands/tests.py (Diff revision 18)
     
     

    If the objective of the test was to make sure shell.bpython wasn't called, it should probably spy on it explicitly using agency.spy_on(shell.bpython) at the start of the test.

  6. rbtools/commands/tests.py (Diff revision 18)
     
     

    Again, did the test intend to call shell.bpython() instead of assigning the classmethod to a variable?

  7. rbtools/commands/tests.py (Diff revision 18)
     
     

    If you add SpyAgency as a parent class, you can use self.spy_on(shell.method_name). This has several advantages: you don't have to create a new SpyAgency for each test, and it will automatically unspy after each test case (which is both less code and more robust if there are unexpected exceptions). Documentation on how to do this can be found in the README at https://github.com/beanbaginc/kgb

  8. rbtools/commands/tests.py (Diff revision 18)
     
     

    Maybe I'm missing something but is there a need to spy on shell.ipython at the end of the test? If not, it's probably a good idea to take this line out.

  9. rbtools/commands/tests.py (Diff revision 18)
     
     

    Same as comment for line 61.

  10. rbtools/commands/tests.py (Diff revision 18)
     
     
     
     

    Again, is the spy operation on shell.bpython repated for a particular reason?

  11. 
      
brennie
  1. 
      
  2. rbtools/commands/tests.py (Diff revision 18)
     
     

    Your test case should inheirit from SpyAgency. Then in unit tests you can use

    self.spy_on(foo, ...)
    

    without having to set up a SpyAgency each time.

  3. 
      
AQ
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. rbtools/commands/shell.py (Diff revision 19)
     
     
    Col: 25
     E128 continuation line under-indented for visual indent
    
  3. rbtools/commands/shell.py (Diff revision 19)
     
     
    Col: 33
     E128 continuation line under-indented for visual indent
    
  4. rbtools/commands/shell.py (Diff revision 19)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  5. rbtools/commands/tests.py (Diff revision 19)
     
     
     'spy_on' imported but unused
    
  6. rbtools/commands/tests.py (Diff revision 19)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  7. rbtools/commands/tests.py (Diff revision 19)
     
     
    Col: 1
     W391 blank line at end of file
    
  8. 
      
AQ
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. rbtools/commands/tests.py (Diff revision 20)
     
     
    Col: 34
     W292 no newline at end of file
    
  3. 
      
AQ
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/tests.py (Diff revision 21)
     
     

    Can you break these up into multiple test cases, e.g.:

    def test_ipython(self):
        # raise SkipTest if we don't have ipython installed.
        pass
    
    def test_ipython_not_installed(self):
        # Ensure that `ipython()` returns `None`.
        pass
    
    # ...
    
    1. I've split up the tests into multiple cases now. I can't really do checks for installation but I think the exception handling in shell.py will take care of that.

  3. rbtools/commands/tests.py (Diff revision 21)
     
     
     
     
     
     
     

    These tests aren't actually testing anything.

    1. My intention for them was to at least test to see that calls were being properly logged by the spy. I looked at other examples in the code base where kgb was used and tried to emulate what I saw there.

    2. Took em out.

  4. rbtools/commands/tests.py (Diff revision 21)
     
     

    shell.ipython can never return None. maybe update the method to return none if it cant get the shell?

  5. rbtools/commands/tests.py (Diff revision 21)
     
     

    These tests dont test the results. See above comments.

  6. rbtools/commands/tests.py (Diff revision 21)
     
     

    These don't test the results of calling log_message.

    1. Is there a way I can do this because log_message just prints?

    2. SpyAgency.spy_on won't work for print(), so we can swap that out with a call to logging.info (or similar) and spy on that.

      When quiet=True logging.info should not be called and when quiet=False it should be.

  7. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. 
      
brennie
  1. 
      
  2. rbtools/commands/shell.py (Diff revision 23)
     
     

    Needs "Returns"

  3. rbtools/commands/shell.py (Diff revision 23)
     
     

    Needs "returns"

  4. rbtools/commands/tests.py (Diff revision 23)
     
     
     

    Blank line between these.

  5. rbtools/commands/tests.py (Diff revision 23)
     
     

    No periods at the end of test cases. Same for all test cases in this file.

  6. rbtools/commands/tests.py (Diff revision 23)
     
     

    You don't need unspy calls at the end of test -- they will automatically be called.

  7. rbtools/commands/tests.py (Diff revision 23)
     
     

    Can you split this into multiple tests?

  8. 
      
AQ
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/shell.py
        rbtools/commands/tests.py
        setup.py
    
    Ignored Files:
        docs/rbtools/rbt/commands/shell.rst
    
    
  2. 
      
AQ
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to dvcs (1cd7a65)
Loading...