functionmodule: automatically find group name#145
Merged
jfilak merged 2 commits intojfilak:masterfrom Mar 28, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds function-group name resolution: passing Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Handler
participant ADT as ADT Resolver
participant Search as ADT Search API
participant Parser as URI Parser
User->>CLI: invoke command with GROUP="-"
CLI->>ADT: resolve_group(connection, function_name)
ADT->>Search: GET /repository/informationsystem/search?query=function_name
Search-->>ADT: XML objectReferences (URIs + types)
ADT->>Parser: group_name_from_uri(result_uri)
Parser-->>ADT: GROUP_NAME (uppercased)
ADT-->>CLI: resolved GROUP_NAME
CLI->>CLI: continue command using resolved GROUP_NAME
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: 3
🤖 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/functionmodule.md`:
- Line 12: Change the "### Group name resolution" heading to level 2 by
replacing "###" with "##" so it becomes "## Group name resolution"; this
preserves proper hierarchy under the top-level "# Function Module" heading and
satisfies markdownlint MD001 for the "Group name resolution" section.
In `@test/unit/test_sap_cli_checkout.py`:
- Around line 573-575: The test fails because sap.adt.FunctionModule is patched
before using its OBJTYPE.code constant, so sap.adt.FunctionModule.OBJTYPE.code
evaluates to a MagicMock; fix by capturing the real constant before patching
(e.g., objcode = sap.adt.FunctionModule.OBJTYPE.code) and then use objcode when
building the fake_walk entries (or alternatively replace
sap.adt.FunctionModule.OBJTYPE.code with the literal 'FUGR/FF'); update the
construction of fake_walk (used to set self.fake_funcgrp.walk) to reference that
captured objcode when creating the SimpleNamespace for fn_modules so the mock
list contains the correct type value.
In `@test/unit/test_sap_cli_function.py`:
- Around line 255-273: In
TestFunctionModuleReadWithDash.test_read_with_dash_group remove the unused and
misleading context variable name `mock_print` from the sys.stdin patch; locate
the line using patch('sys.stdin', StringIO('')) (within the test method that
calls fm_parse_args and args.execute) and change the context manager to either
omit the "as mock_print" binding (i.e., "with patch('sys.stdin',
StringIO('')):") or bind it to "_" if a placeholder is required.
🪄 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: 149c9388-9cb2-47ea-a72d-4cd695e3e176
📒 Files selected for processing (7)
doc/commands/functionmodule.mdsap/adt/function.pysap/cli/function.pytest/unit/fixtures_adt_function.pytest/unit/test_sap_adt_function.pytest/unit/test_sap_cli_checkout.pytest/unit/test_sap_cli_function.py
filak-sap
force-pushed
the
resolve_function_group
branch
from
da8fcbb to
214fe09
Compare
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 `@sap/adt/function.py`:
- Around line 140-159: The group_name_from_uri function currently hardcodes the
ADT root and returns an empty name for URIs like .../functions/groups//...;
update group_name_from_uri to locate the substring f"{cls.OBJTYPE.basepath}/"
anywhere in the given uri (e.g. using uri.find or split on that marker) instead
of requiring a fixed "/sap/bc/adt/" prefix, then extract the segment immediately
after that marker as the group name, validate that the marker exists and that
the extracted group_name is non-empty, and raise sap.errors.SAPCliError if the
marker is not found or the group name is empty; keep references to
cls.OBJTYPE.basepath and the classmethod group_name_from_uri so callers like
resolve_group() work with non-standard ADT roots.
🪄 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: 7cef2297-dabf-45b5-b9f1-383370b3f327
📒 Files selected for processing (8)
doc/commands/functionmodule.mdsap/adt/function.pysap/cli/checkout.pysap/cli/function.pytest/unit/fixtures_adt_function.pytest/unit/test_sap_adt_function.pytest/unit/test_sap_cli_checkout.pytest/unit/test_sap_cli_function.py
✅ Files skipped from review due to trivial changes (1)
- doc/commands/functionmodule.md
🚧 Files skipped from review as they are similar to previous changes (4)
- test/unit/test_sap_cli_checkout.py
- test/unit/test_sap_cli_function.py
- sap/cli/function.py
- test/unit/test_sap_adt_function.py
All of the standard tools allow me to open function module without passing function group. It looks like that function module names have global namespace again. I only need to pass function group when creating a new function module. This patch introduces the possibility to pass '-' instead of function group in function module commands. That keeps backward compatibility and allows faster processing (if you leave function group out, sapcli must do one extra HTTP round trip to find it).
To make it it crystal clear where the code handling the object types is located. Not changed in the tests, there we do not want too much of dynamic behavior. Also you can see the exercise with patch and copying OBJTYPE - the patch replaced also OBJTYPE and that made the tests less valuable - thanks to mistakenly pushed changed CI discovered the problem and I decided to keep test using literals for object codes to get closer to real ADT responses.
filak-sap
force-pushed
the
resolve_function_group
branch
from
214fe09 to
9d2faed
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/test_sap_adt_function.py (1)
21-24: Align fixture module naming with test-fixture convention.The new fixtures are imported from
fixtures_adt_function, but fixture modules are expected to follow thetest_...naming convention for unit-test fixtures. Please rename the fixture module and update imports to match the repository rule.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_adt_function.py` around lines 21 - 24, Rename the fixture module to use the required test_ prefix and match the tested module name, then update the import in test/unit/test_sap_adt_function.py: change the source module currently imported as fixtures_adt_function to the new test-prefixed fixture module name and keep the existing fixture symbols (FUNCTION_MODULE_CODE_ABAPGIT, SEARCH_FUNCTION_MODULE_RESPONSE_XML, SEARCH_FUNCTION_MODULE_NOT_FOUND_RESPONSE_XML, SEARCH_FUNCTION_MODULE_WRONG_TYPE_RESPONSE_XML) unchanged; ensure the fixture file name follows the "test_<something>_sap_adt_function" convention so it aligns with the tested module and repository rules, and update any references/imports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/test_sap_adt_function.py`:
- Around line 21-24: Rename the fixture module to use the required test_ prefix
and match the tested module name, then update the import in
test/unit/test_sap_adt_function.py: change the source module currently imported
as fixtures_adt_function to the new test-prefixed fixture module name and keep
the existing fixture symbols (FUNCTION_MODULE_CODE_ABAPGIT,
SEARCH_FUNCTION_MODULE_RESPONSE_XML,
SEARCH_FUNCTION_MODULE_NOT_FOUND_RESPONSE_XML,
SEARCH_FUNCTION_MODULE_WRONG_TYPE_RESPONSE_XML) unchanged; ensure the fixture
file name follows the "test_<something>_sap_adt_function" convention so it
aligns with the tested module and repository rules, and update any
references/imports accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fce4217-6bc9-4411-a15a-09a5cd47213d
📒 Files selected for processing (8)
doc/commands/functionmodule.mdsap/adt/function.pysap/cli/checkout.pysap/cli/function.pytest/unit/fixtures_adt_function.pytest/unit/test_sap_adt_function.pytest/unit/test_sap_cli_checkout.pytest/unit/test_sap_cli_function.py
✅ Files skipped from review due to trivial changes (1)
- sap/adt/function.py
🚧 Files skipped from review as they are similar to previous changes (6)
- test/unit/test_sap_cli_checkout.py
- test/unit/fixtures_adt_function.py
- doc/commands/functionmodule.md
- sap/cli/checkout.py
- test/unit/test_sap_cli_function.py
- sap/cli/function.py
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
2 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.
All of the standard tools allow me to open function module without passing function group. It looks like that function module names have global namespace again. I only need to pass function group when creating a new function module.
This patch introduces the possibility to pass '-' instead of function group in function module commands. That keeps backward compatibility and allows faster processing (if you leave function group out, sapcli must do one extra HTTP round trip to find it).