feat: support executables passed to ipdb3#284
feat: support executables passed to ipdb3#284iloveitaly wants to merge 1 commit intogotcha:masterfrom
Conversation
For instance, if your project has a script section a bin/ entry in your venv will be generated. It's helpful to be able to pass the command reference to ipdb for debugging.
|
@gotcha reminder here! Would love to have this merged in. |
|
@gotcha ping! |
|
@gotcha ping on this! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe change adds support for resolving executables from the system PATH when Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ipdb/__main__.py`:
- Around line 304-311: The code calls which(mainpyfile) and then passes
mainpyfile to os.path.exists without checking for None; update the logic around
the which() call so you only reassign mainpyfile if which(...) returned a
non-None path (or otherwise handle a None return) before calling
os.path.exists(mainpyfile), and ensure the error path (the print and sys.exit(1)
after the run_as_module check) handles the None case cleanly; also fix the
comment from "a executable" to "an executable". Use the existing symbols
mainpyfile, which(...), run_as_module, os.path.exists, print(...), and sys.exit
to locate where to change the code.
|
|
||
| # a executable command in $PATH may be passed | ||
| if not os.path.exists(mainpyfile): | ||
| mainpyfile = which(mainpyfile) | ||
|
|
||
| if not run_as_module and not os.path.exists(mainpyfile): | ||
| print("Error:", mainpyfile, "does not exist") | ||
| sys.exit(1) |
There was a problem hiding this comment.
which() returning None causes TypeError crash.
When the provided argument is neither an existing file nor a valid executable in PATH, which() returns None. This causes os.path.exists(mainpyfile) on line 309 to raise TypeError since os.path.exists() does not accept None.
Additionally, the comment has a minor grammar issue: "a executable" → "an executable".
🐛 Proposed fix to handle None from which()
mainpyfile = args[0] # Get script filename
- # a executable command in $PATH may be passed
+ # an executable command in $PATH may be passed
if not os.path.exists(mainpyfile):
- mainpyfile = which(mainpyfile)
+ resolved = which(mainpyfile)
+ if resolved is not None:
+ mainpyfile = resolved
if not run_as_module and not os.path.exists(mainpyfile):
print("Error:", mainpyfile, "does not exist")
sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # a executable command in $PATH may be passed | |
| if not os.path.exists(mainpyfile): | |
| mainpyfile = which(mainpyfile) | |
| if not run_as_module and not os.path.exists(mainpyfile): | |
| print("Error:", mainpyfile, "does not exist") | |
| sys.exit(1) | |
| # an executable command in $PATH may be passed | |
| if not os.path.exists(mainpyfile): | |
| resolved = which(mainpyfile) | |
| if resolved is not None: | |
| mainpyfile = resolved | |
| if not run_as_module and not os.path.exists(mainpyfile): | |
| print("Error:", mainpyfile, "does not exist") | |
| sys.exit(1) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ipdb/__main__.py` around lines 304 - 311, The code calls which(mainpyfile)
and then passes mainpyfile to os.path.exists without checking for None; update
the logic around the which() call so you only reassign mainpyfile if which(...)
returned a non-None path (or otherwise handle a None return) before calling
os.path.exists(mainpyfile), and ensure the error path (the print and sys.exit(1)
after the run_as_module check) handles the None case cleanly; also fix the
comment from "a executable" to "an executable". Use the existing symbols
mainpyfile, which(...), run_as_module, os.path.exists, print(...), and sys.exit
to locate where to change the code.
2 participants
For instance, if your project has a script section a bin/ entry in your venv
will be generated. It's helpful to be able to pass the command reference to
ipdb for debugging.
Summary by CodeRabbit
Release Notes