Open
Conversation
* add ipython-profile support * add tests to fetch ipytohn_profiler from config --------- Co-authored-by: Wouter Vanden Hove <wvandenhove@ekglobal.co>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #266 +/- ##
==========================================
- Coverage 81.64% 81.02% -0.62%
==========================================
Files 7 7
Lines 512 564 +52
==========================================
+ Hits 418 457 +39
- Misses 94 107 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gotcha
reviewed
Mar 22, 2023
gotcha
reviewed
Mar 22, 2023
gotcha
reviewed
Mar 22, 2023
Owner
|
Thanks for this PR. |
* add ipython-profile support * add tests to fetch ipytohn_profiler from config * cleanup profile-tests --------- Co-authored-by: Wouter Vanden Hove <wvandenhove@ekglobal.co>
ulgens
approved these changes
Feb 12, 2024
Owner
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request adds support for non-default IPython profiles in ipdb. Users can specify a profile via the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ipdb as ipdb/__main__.py
participant Config as Config/Env
participant ProfileDir as IPython ProfileDir
participant IPythonApp as TerminalIPythonApp
User->>ipdb: _init_pdb(ipython_profile=None)
ipdb->>Config: get_ipython_profile_from_config()
Config-->>ipdb: profile_name ("default" or configured)
ipdb->>ipdb: _get_debugger_cls(ipython_profile)
ipdb->>ProfileDir: find_profile_dir_by_name(profile_name)
alt Profile found
ProfileDir-->>ipdb: profile_dir
else Profile not found
ProfileDir-->>ipdb: ProfileDirError → fallback to "default"
end
ipdb->>IPythonApp: TerminalIPythonApp(profile_dir=resolved_dir)
IPythonApp-->>ipdb: app instance
ipdb-->>User: debugger initialized with selected profile
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 4
🧹 Nitpick comments (1)
tests/test_config.py (1)
427-470: Use unittest assertions instead of bareassert.Bare asserts can be skipped under optimized execution (python -O) and are inconsistent with the extensive use of
self.assertEqual()and other unittest assertion methods throughout this test file.Proposed fix
with ModifiedEnvironment(IPDB_CONFIG=None, HOME=self.tmpd): profile_name = get_ipython_profile_from_config() - assert profile_name == "default" + self.assertEqual("default", profile_name) @@ with ModifiedEnvironment(IPDB_CONFIG=None, HOME=self.tmpd): profile_name = get_ipython_profile_from_config() - assert profile_name == "default" + self.assertEqual("default", profile_name) @@ with ModifiedEnvironment(IPDB_CONFIG=None, HOME=self.tmpd): profile_name = get_ipython_profile_from_config() - assert profile_name == "foo" + self.assertEqual("foo", profile_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_config.py` around lines 427 - 470, Replace bare Python asserts in the tests with unittest assertion methods: in test_default_profile_setup and test_non_default_profile_setup (and the nearby test that calls get_ipython_profile_from_config inside ModifiedEnvironment), change lines like assert profile_name == "default" and assert profile_name == "foo" to self.assertEqual(profile_name, "default") and self.assertEqual(profile_name, "foo") respectively so the tests use the TestCase assertions consistently (functions referenced: get_ipython_profile_from_config, ModifiedEnvironment, write_lines_to_file).
🤖 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 65-72: The _init_pdb function currently uses a mutable default
commands=[] and eagerly evaluates get_ipython_profile_from_config() inside
os.getenv; change the signature to def _init_pdb(context=None,
ipython_profile=None, commands=None): set commands = [] inside the function if
commands is None, and avoid passing get_ipython_profile_from_config() directly
to os.getenv by first checking os.getenv("IPDB_IPYTHON_PROFILE", None) and only
calling get_ipython_profile_from_config() when that env var is not set (do the
same pattern for context: check os.getenv("IPDB_CONTEXT_SIZE", None) and call
get_context_from_config() only if env is None) so config parsing remains lazy
and the default list is not shared across calls, referencing _init_pdb,
commands, ipython_profile, get_ipython_profile_from_config,
get_context_from_config, and os.getenv.
In `@README.rst`:
- Around line 127-132: The README has a typo in the environment variable name:
replace the incorrect string "IPDB_IPYTON_PROFILE" with the correct
"IPDB_IPYTHON_PROFILE" in the code block and any related mentions so the docs
match runtime behavior (search for the literal IPDB_IPYTON_PROFILE in the
README.rst and update it accordingly).
- Line 140: Fix the malformed inline-literal in README.rst by removing the extra
backtick after ``profile_ipdb``` and inserting a space before "in your
``IPYTHON_HOME``", so the sentence reads with a single inline-literal
``profile_ipdb`` and a space: "This should correspond with a profile-directory
``profile_ipdb`` in your ``IPYTHON_HOME``." Reference the literal text
"profile_ipdb" and "IPYTHON_HOME" to locate the change.
In `@tests/test_config.py`:
- Around line 141-156: Remove the duplicated test method definition for
test_noenv_nodef_nosetup_pyproject so only a single test function remains;
locate both definitions of test_noenv_nodef_nosetup_pyproject in the
tests/test_config.py file, decide which body to keep (or merge assertions from
both if they differ), delete the redundant def, and ensure imports and
references such as get_config, ModifiedEnvironment, env_filename,
default_filename, and setup_filename remain correct and the test suite still
passes.
---
Nitpick comments:
In `@tests/test_config.py`:
- Around line 427-470: Replace bare Python asserts in the tests with unittest
assertion methods: in test_default_profile_setup and
test_non_default_profile_setup (and the nearby test that calls
get_ipython_profile_from_config inside ModifiedEnvironment), change lines like
assert profile_name == "default" and assert profile_name == "foo" to
self.assertEqual(profile_name, "default") and self.assertEqual(profile_name,
"foo") respectively so the tests use the TestCase assertions consistently
(functions referenced: get_ipython_profile_from_config, ModifiedEnvironment,
write_lines_to_file).
Comment on lines
+65
to
+72
| def _init_pdb(context=None, ipython_profile=None, commands=[]): | ||
| if context is None: | ||
| context = os.getenv("IPDB_CONTEXT_SIZE", get_context_from_config()) | ||
| debugger_cls = _get_debugger_cls() | ||
|
|
||
| if ipython_profile is None: | ||
| ipython_profile = os.getenv( | ||
| "IPDB_IPYTHON_PROFILE", get_ipython_profile_from_config() | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the patterns exist in the _init_pdb function
echo "=== Checking for mutable default argument pattern ==="
rg -n 'def _init_pdb\(context=None, ipython_profile=None, commands=\[\]\):' ipdb/__main__.py
echo "=== Checking for eager evaluation pattern ==="
rg -n 'IPDB_IPYTHON_PROFILE", get_ipython_profile_from_config\(\)' ipdb/__main__.py
echo "=== Reading the actual function to see context ==="
rg -n -A 20 'def _init_pdb' ipdb/__main__.py | head -40Repository: gotcha/ipdb
Length of output: 1036
Fix mutable default argument and eager config evaluation in _init_pdb.
commands=[] as a default argument is a mutable default anti-pattern that causes the list to be shared across all function calls. Additionally, get_ipython_profile_from_config() is evaluated immediately on every call to os.getenv(), even when IPDB_IPYTHON_PROFILE is set, which defeats the lazy evaluation intent and can trigger unnecessary config parsing.
Proposed fix
-def _init_pdb(context=None, ipython_profile=None, commands=[]):
+def _init_pdb(context=None, ipython_profile=None, commands=None):
+ if commands is None:
+ commands = []
if context is None:
context = os.getenv("IPDB_CONTEXT_SIZE", get_context_from_config())
if ipython_profile is None:
- ipython_profile = os.getenv(
- "IPDB_IPYTHON_PROFILE", get_ipython_profile_from_config()
- )
+ ipython_profile = os.getenv("IPDB_IPYTHON_PROFILE")
+ if ipython_profile is None:
+ ipython_profile = get_ipython_profile_from_config()🧰 Tools
🪛 Ruff (0.15.2)
[warning] 65-65: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ipdb/__main__.py` around lines 65 - 72, The _init_pdb function currently uses
a mutable default commands=[] and eagerly evaluates
get_ipython_profile_from_config() inside os.getenv; change the signature to def
_init_pdb(context=None, ipython_profile=None, commands=None): set commands = []
inside the function if commands is None, and avoid passing
get_ipython_profile_from_config() directly to os.getenv by first checking
os.getenv("IPDB_IPYTHON_PROFILE", None) and only calling
get_ipython_profile_from_config() when that env var is not set (do the same
pattern for context: check os.getenv("IPDB_CONTEXT_SIZE", None) and call
get_context_from_config() only if env is None) so config parsing remains lazy
and the default list is not shared across calls, referencing _init_pdb,
commands, ipython_profile, get_ipython_profile_from_config,
get_context_from_config, and os.getenv.
Comment on lines
+127
to
+132
| You can set a non-default profile by setting the environment variable ``IPDB_IPYTON_PROFILE``: | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| export IPDB_IPYTON_PROFILE="ipdb" | ||
|
|
There was a problem hiding this comment.
Fix env-var name typo in docs (IPDB_IPYTHON_PROFILE).
IPDB_IPYTON_PROFILE (missing H) will not match runtime behavior, so users following docs won’t activate the feature.
Proposed fix
-You can set a non-default profile by setting the environment variable ``IPDB_IPYTON_PROFILE``:
+You can set a non-default profile by setting the environment variable ``IPDB_IPYTHON_PROFILE``:
- export IPDB_IPYTON_PROFILE="ipdb"
+ export IPDB_IPYTHON_PROFILE="ipdb"📝 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.
Suggested change
| You can set a non-default profile by setting the environment variable ``IPDB_IPYTON_PROFILE``: | |
| .. code-block:: bash | |
| export IPDB_IPYTON_PROFILE="ipdb" | |
| You can set a non-default profile by setting the environment variable ``IPDB_IPYTHON_PROFILE``: | |
| .. code-block:: bash | |
| export IPDB_IPYTHON_PROFILE="ipdb" | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.rst` around lines 127 - 132, The README has a typo in the environment
variable name: replace the incorrect string "IPDB_IPYTON_PROFILE" with the
correct "IPDB_IPYTHON_PROFILE" in the code block and any related mentions so the
docs match runtime behavior (search for the literal IPDB_IPYTON_PROFILE in the
README.rst and update it accordingly).
| [tool.ipdb] | ||
| ipython_profile = "ipdb" | ||
|
|
||
| This should correspond with a profile-directory ``profile_ipdb```in your ``IPYTHON_HOME``. |
There was a problem hiding this comment.
Fix malformed inline-literal markup.
The profile directory literal has an extra backtick and missing space.
Proposed fix
-This should correspond with a profile-directory ``profile_ipdb```in your ``IPYTHON_HOME``.
+This should correspond with a profile-directory ``profile_ipdb`` in your ``IPYTHON_HOME``.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.rst` at line 140, Fix the malformed inline-literal in README.rst by
removing the extra backtick after ``profile_ipdb``` and inserting a space before
"in your ``IPYTHON_HOME``", so the sentence reads with a single inline-literal
``profile_ipdb`` and a space: "This should correspond with a profile-directory
``profile_ipdb`` in your ``IPYTHON_HOME``." Reference the literal text
"profile_ipdb" and "IPYTHON_HOME" to locate the change.
Comment on lines
+141
to
+156
| def test_noenv_nodef_nosetup_pyproject(self): | ||
| """ | ||
| Setup: $IPDB_CONFIG unset, $HOME/.ipdb does not exist, | ||
| setup.cfg does not exist, pyproject.toml exists | ||
| Result: load pyproject.toml | ||
| """ | ||
| os.unlink(self.env_filename) | ||
| os.unlink(self.default_filename) | ||
| os.remove(self.setup_filename) | ||
| with ModifiedEnvironment(IPDB_CONFIG=None, HOME=self.tmpd): | ||
| cfg = get_config() | ||
| # breakpoint() | ||
| self.assertEqual(["ipdb"], cfg.sections()) | ||
| self.assertEqual(self.pyproject_context, cfg.getint("ipdb", "context")) | ||
| self.assertRaises(configparser.NoOptionError, cfg.get, "ipdb", "version") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify duplicate method definitions.
rg -n 'def test_noenv_nodef_nosetup_pyproject\(' tests/test_config.pyRepository: gotcha/ipdb
Length of output: 162
Remove duplicated test method definition.
test_noenv_nodef_nosetup_pyproject is defined twice (Lines 126 and 141); the second definition overrides the first and silently drops one test body.
Proposed fix
- def test_noenv_nodef_nosetup_pyproject(self):
- """
- Setup: $IPDB_CONFIG unset, $HOME/.ipdb does not exist,
- setup.cfg does not exist, pyproject.toml exists
- Result: load pyproject.toml
- """
- os.unlink(self.env_filename)
- os.unlink(self.default_filename)
- os.remove(self.setup_filename)
- with ModifiedEnvironment(IPDB_CONFIG=None, HOME=self.tmpd):
- cfg = get_config()
- # breakpoint()
- self.assertEqual(["ipdb"], cfg.sections())
- self.assertEqual(self.pyproject_context, cfg.getint("ipdb", "context"))
- self.assertRaises(configparser.NoOptionError, cfg.get, "ipdb", "version")📝 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.
Suggested change
| def test_noenv_nodef_nosetup_pyproject(self): | |
| """ | |
| Setup: $IPDB_CONFIG unset, $HOME/.ipdb does not exist, | |
| setup.cfg does not exist, pyproject.toml exists | |
| Result: load pyproject.toml | |
| """ | |
| os.unlink(self.env_filename) | |
| os.unlink(self.default_filename) | |
| os.remove(self.setup_filename) | |
| with ModifiedEnvironment(IPDB_CONFIG=None, HOME=self.tmpd): | |
| cfg = get_config() | |
| # breakpoint() | |
| self.assertEqual(["ipdb"], cfg.sections()) | |
| self.assertEqual(self.pyproject_context, cfg.getint("ipdb", "context")) | |
| self.assertRaises(configparser.NoOptionError, cfg.get, "ipdb", "version") |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 141-141: Redefinition of unused test_noenv_nodef_nosetup_pyproject from line 126: test_noenv_nodef_nosetup_pyproject redefined here
(F811)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_config.py` around lines 141 - 156, Remove the duplicated test
method definition for test_noenv_nodef_nosetup_pyproject so only a single test
function remains; locate both definitions of test_noenv_nodef_nosetup_pyproject
in the tests/test_config.py file, decide which body to keep (or merge assertions
from both if they differ), delete the redundant def, and ensure imports and
references such as get_config, ModifiedEnvironment, env_filename,
default_filename, and setup_filename remain correct and the test suite still
passes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
IPDB_IPYTHON_PROFILEenvironment variable or configuring it via configuration file, with fallback to default profile if the profile directory doesn't exist.Documentation
Tests