Skip to content

Conversation

@isakigus
Copy link

I use this library nearly daily and I just wanted to add this debugger for having debugger command history when I debug.

@tamentis
Copy link
Owner

Thank you for your pull request, it's much appreciated. Here are a few comments:

  1. Use snake-case for function parameters (ipython=True)
  2. IPython shouldn't be a requirement for this module, as implemented rpdb will crash when installed on a fresh installation since it will be missing IPython.
  3. Since you have a lot of repetition in this for no good reason (IRpdb and Rpdb are basically identical), I'd suggest wrapping your import in a try/except and keep everything in a single class.

+ snake case use
+ refactor code to one class
+ import protected with try/catch 
+ thank for all the comments
@isakigus
Copy link
Author

isakigus commented Sep 19, 2018

Thank you for the response and valuable comments. I fixed all the point raised in the first PR submission.

Copy link
Owner

@tamentis tamentis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't have time to test that now, but here are some comments.

README.rst Outdated

For using IPython debugger

import rpdb; rpdb.set_trace(IPython=True)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be lower-cased too.

rpdb/__init__.py Outdated
from IPython.core.debugger import Pdb
IPYTHON_ENABLE = True
except ImportError:
IPYTHON_ENABLE = False
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ENABLE as much as it is AVAILABLE, also I don't think this should be all upper-cased since this is more variable than it is constant.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I would probably import it as IPdb to avoid confusion lower in the file.

rpdb/__init__.py Outdated
self.old_stdin = sys.stdin
self.port = port

self.debugger = Pdb if ipython and IPYTHON_ENABLE else pdb.Pdb
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone has ipython == True but the check variable is False, this is silently going to revert to Pdb, I'm okay with that, but I think it should be in the README.

@nikoren
Copy link

nikoren commented Apr 15, 2019

@tamentis is README the only blocker for this PR? I can help with that , it is very useful option

@tamentis
Copy link
Owner

I tried both Python 3.7 and 2.7 and couldn't get this to work. Am I doing anything wrong?

Test script (test.py):

#!/usr/bin/env python

import rpdb; rpdb.set_trace(ipython=True)

Terminal 1:

$ cd rpdb
$ virtualenv venv
$ . ./venv/bin/activate
$ pip install -e .
$ pip install ipdb
$ python test.py
pdb is running on 127.0.0.1:4444

Terminal 2:

$ nc localhost 4444
--Return--
> /private/tmp/test.py(4)<module>()->None
-> import rpdb; rpdb.set_trace(ipython=True)
(Pdb) 

@vthorey
Copy link

vthorey commented Jul 29, 2021

It works for me with python 3.6.10

(pydss) ➜  ~ nc localhost 4444
--Return--
None
> /Users/val/testipython.py(3)<module>()
      1 #!/usr/bin/env python
      2
----> 3 import rpdb; rpdb.set_trace(ipython=True)

ipdb>

However, auto-completion does not work ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants