Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py

Col: 6
 E113 unexpected indentation

Col: 6
 E111 indentation is not a multiple of four

Col: 1
 E302 expected 2 blank lines, found 1

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

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py

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)

Blank line between these.

Tool: Pyflakes
Processed Files:
    rbtools/commands/shell.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py

 'logging' imported but unused

 'Option' imported but unused

 'LogLevelFilter' imported but unused

 'OptionGroup' imported but unused

 'get_config_paths' imported but unused

 'load_config' imported but unused

 'parse_config_file' imported but unused

 'execute' imported but unused

 'die' imported but unused

Col: 32
 E231 missing whitespace after ':'

Col: 34
 E231 missing whitespace after ','

Col: 38
 E231 missing whitespace after ':'

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

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

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

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

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

Single quotes for strings.

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

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py

Col: 30
 E225 missing whitespace around operator

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

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

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

Tool: Pyflakes
Processed Files:
    rbtools/commands/shell.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py

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

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py



Tool: Pyflakes
Processed Files:
    rbtools/commands/shell.py
    setup.py

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

Should use sentence casing. Needs a period.

Parens not necessary here.

Parens not necessary here.

Tool: Pyflakes
Processed Files:
    rbtools/commands/shell.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py

 'RBClient' imported but unused

 'CONFIG_FILE' imported but unused

Col: 1
 E302 expected 2 blank lines, found 1

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

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

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

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

 local variable 'e' is assigned to but never used

Tool: Pyflakes
Processed Files:
    rbtools/commands/shell.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py

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

Tool: Pyflakes
Processed Files:
    rbtools/commands/shell.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py

This is looking pretty great!

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.

No blank line necessary here.

This is a little cleaner as:

except Exception as e:
    logging.error('Could not run script "%s" for RBTools shell: %s",
                  script_path, e)

You can skip the conditional if you do this instead;

for import_command in config.get('SHELL_IMPORTS', []):

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.

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py



Tool: Pyflakes
Processed Files:
    rbtools/commands/shell.py
    setup.py

 'logging' imported but unused

 'os' imported but unused

 'sys' imported but unused

 'Option' imported but unused

Tool: Pyflakes
Processed Files:
    rbtools/commands/shell.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py

 'load_config' imported but unused

Col: 1
 E302 expected 2 blank lines, found 1

Col: 9
 E124 closing bracket does not match visual indentation

Col: 9
 E124 closing bracket does not match visual indentation

Tool: Pyflakes
Processed Files:
    rbtools/commands/shell.py
    setup.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py

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.

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.

"pre-configured"

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.

I feel like we should have

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


so that we don't pollute namespace.

Not necessary.

Not necessary.

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.)

Not necessary.

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

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/shell.py
    setup.py



Tool: Pyflakes
Processed Files:
    rbtools/commands/shell.py
    setup.py

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

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

Doesn't need parentheses.

Use a dict literal:

namespace = {
    'config': self.config,
    'options': vars(self.options),
}

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

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

Same comment about -q.

This should probably be:

elif shell


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

I dont think we need this.

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

In your description and testing done, it should be "Python" and "IPython".

Python

Suppress

Docstring w/ args?

Blank line between these.

Blank line between these.

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

Ship It!

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

 'spy_on' imported but unused

Col: 1
 E302 expected 2 blank lines, found 1

 local variable 'ipython_module' is assigned to but never used

 local variable 'bpython_module' is assigned to but never used

 local variable 'bpython_module' is assigned to but never used

 local variable 'ipython_module' is assigned to but never used

 local variable 'client' is assigned to but never used

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.

It's really cool that your shell has so many configuration options!

It's super cool that you could smartly re-use existing shell implementations for your project!

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

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.

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

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

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.

Same as comment for line 61.

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

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.

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

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

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

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

 'spy_on' imported but unused

Col: 1
 E302 expected 2 blank lines, found 1

Col: 1
 W391 blank line at end of file

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

Col: 34
 W292 no newline at end of file

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

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

# ...

These tests aren't actually testing anything.

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

These tests dont test the results. See above comments.

These don't test the results of calling log_message.