Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as ABAP CLI
participant ADTSearch as ADTSearch
participant Connection as ADT Connection
participant TableWriter as TableWriter
User->>CLI: sapcli abap find [--max-results N] TERM
CLI->>CLI: validate args\ntrim TERM\nappend '*' if missing
CLI->>ADTSearch: new ADTSearch(connection)
CLI->>ADTSearch: quick_search(query=TERM*, max_results=N)
ADTSearch->>Connection: GET /repository/objectRepository/... ?operation=quickSearch&query=...
Connection-->>ADTSearch: return search XML / references
ADTSearch-->>CLI: parsed results.references
CLI->>TableWriter: write rows (typ, name, description)
TableWriter->>User: display formatted table
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
test/unit/test_sap_cli_abap.py (1)
18-38: Move new XML fixtures into a dedicated fixture module.At Line 18-38, fixtures are embedded directly in the test module. Please move them to a fixture module under
test/unit/with atest_-prefixed filename aligned to the tested module naming convention.As per coding guidelines: "Test fixtures should be stored as module files with the prefix test_" and "Test fixture file names should have suffixes matching the tested module in the same way as test files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/test_sap_cli_abap.py` around lines 18 - 38, The two XML fixture strings FIXTURE_SEARCH_RESPONSE_TWO_RESULTS and FIXTURE_SEARCH_RESPONSE_EMPTY are embedded in the test module; extract them into a dedicated fixture module (create a new module with a test_ prefix, e.g. test_sap_cli_abap_fixtures.py) under the same test package, keep the constant names unchanged, and update test_sap_cli_abap.py to import these constants from the new fixture module; ensure the new fixture module name follows the test_ prefix and naming convention matching the tested module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/commands/abap.md`:
- Around line 26-31: The fenced code block containing the table lacks a language
tag (triggers MD040); update the fenced block around the table so it begins with
a language identifier (e.g., ```text) instead of just ``` to satisfy
markdownlint; look for the fenced block that contains the "Object type | Name |
Description" table and add the language tag to the opening fence.
In `@sap/cli/abap.py`:
- Around line 62-74: The find command should validate CLI inputs before calling
sap.adt.search.ADTSearch.quick_search: check that args.max_results > 0 and that
args.term is not empty/only whitespace (use term = args.term.strip() before
appending '*'); if invalid, raise an exception subclass of
sap.errors.SAPCliError with a clear message (e.g., for max_results and for empty
term) so the CLI entrypoint handles the error nicely; perform these checks in
the find function before creating ADTSearch or calling quick_search.
In `@test/unit/test_sap_cli_abap.py`:
- Line 11: Remove the unused Request import from the test import line so the
file imports only the actually used symbols (e.g., change the "from mock import
BufferConsole, Connection, Response, Request" import to omit Request); update
the import statement that references BufferConsole, Connection, Response to
ensure Request is not imported and re-run linting to confirm F401 is resolved.
- Line 280: The unpacked variable console from make_console_factory() is unused;
replace it with an underscore to signal intentional unused variable. Update each
call where you currently do "console, factory = make_console_factory()" (at the
four locations) to "_ , factory = make_console_factory()" (or "_, factory =
make_console_factory()") and ensure args.console_factory still receives factory;
the function name make_console_factory and the assignment to
args.console_factory identify where to change.
---
Nitpick comments:
In `@test/unit/test_sap_cli_abap.py`:
- Around line 18-38: The two XML fixture strings
FIXTURE_SEARCH_RESPONSE_TWO_RESULTS and FIXTURE_SEARCH_RESPONSE_EMPTY are
embedded in the test module; extract them into a dedicated fixture module
(create a new module with a test_ prefix, e.g. test_sap_cli_abap_fixtures.py)
under the same test package, keep the constant names unchanged, and update
test_sap_cli_abap.py to import these constants from the new fixture module;
ensure the new fixture module name follows the test_ prefix and naming
convention matching the tested module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36451734-f4c0-4220-986c-37a43fd0724a
📒 Files selected for processing (3)
doc/commands/abap.mdsap/cli/abap.pytest/unit/test_sap_cli_abap.py
| ``` | ||
| Object type | Name | Description | ||
| ------------|-------------|---------------------------- | ||
| TTYP/DA | BAPIRET2_T | Return parameter table | ||
| TABL/DS | BAPIRET2_T1 | Proxy Structure (generated) | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced output block.
At Line 26, markdownlint MD040 is triggered because the code fence has no language.
Suggested fix
-```
+```text
Object type | Name | Description
------------|-------------|----------------------------
TTYP/DA | BAPIRET2_T | Return parameter table
TABL/DS | BAPIRET2_T1 | Proxy Structure (generated)</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @doc/commands/abap.md around lines 26 - 31, The fenced code block containing
the table lacks a language tag (triggers MD040); update the fenced block around
the table so it begins with a language identifier (e.g., text) instead of just to satisfy markdownlint; look for the fenced block that contains the
"Object type | Name | Description" table and add the language tag to the opening
fence.
</details>
<!-- fingerprinting:phantom:poseidon:hawk:7aa1ca92-7dcf-4068-89ab-ae9ee4eb03bd -->
<!-- This is an auto-generated comment by CodeRabbit -->
test/unit/test_sap_cli_abap.py
Outdated
| import sap.platform.abap.run | ||
|
|
||
| from mock import BufferConsole, Connection | ||
| from mock import BufferConsole, Connection, Response, Request |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether Request is referenced beyond the import line.
rg -nP '\bRequest\b' --type=py test/unit/test_sap_cli_abap.pyRepository: jfilak/sapcli
Length of output: 121
Remove unused Request import to fix lint failure.
At Line 11, Request is imported but never referenced in the file (F401).
Suggested fix
-from mock import BufferConsole, Connection, Response, Request
+from mock import BufferConsole, Connection, Response📝 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.
| from mock import BufferConsole, Connection, Response, Request | |
| from mock import BufferConsole, Connection, Response |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 11-11: 'mock.Request' imported but unused
(F401)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/test_sap_cli_abap.py` at line 11, Remove the unused Request import
from the test import line so the file imports only the actually used symbols
(e.g., change the "from mock import BufferConsole, Connection, Response,
Request" import to omit Request); update the import statement that references
BufferConsole, Connection, Response to ensure Request is not imported and re-run
linting to confirm F401 is resolved.
test/unit/test_sap_cli_abap.py
Outdated
| Response(text=FIXTURE_SEARCH_RESPONSE_EMPTY, status_code=200), | ||
| ]) | ||
| args = parse_args(['find', 'BAPIRET2_T']) | ||
| console, factory = make_console_factory() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and its total line count
wc -l test/unit/test_sap_cli_abap.pyRepository: jfilak/sapcli
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check the specific lines mentioned in the review comment
sed -n '275,325p' test/unit/test_sap_cli_abap.py | cat -nRepository: jfilak/sapcli
Length of output: 2407
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of make_console_factory to understand the pattern
rg "make_console_factory" test/unit/test_sap_cli_abap.py -B 2 -A 2 -nRepository: jfilak/sapcli
Length of output: 3038
Replace unused console variables with _ at lines 280, 292, 304, and 321.
The console variable is unpacked from make_console_factory() but never used, only factory is assigned to args.console_factory. Use the underscore convention to indicate intentionally unused variables (RUF059).
Suggested fix
- console, factory = make_console_factory()
+ _, factory = make_console_factory()Apply this change at lines 280, 292, 304, and 321.
📝 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.
| console, factory = make_console_factory() | |
| _, factory = make_console_factory() |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 280-280: Unpacked variable console is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/test_sap_cli_abap.py` at line 280, The unpacked variable console
from make_console_factory() is unused; replace it with an underscore to signal
intentional unused variable. Update each call where you currently do "console,
factory = make_console_factory()" (at the four locations) to "_ , factory =
make_console_factory()" (or "_, factory = make_console_factory()") and ensure
args.console_factory still receives factory; the function name
make_console_factory and the assignment to args.console_factory identify where
to change.
This patch just exposes the ADT search functionality which already exists in the code base as a new command.
2 participants
This patch just exposes the ADT search functionality which already exists in the code base as a new command.