Add NORMILIZE_GIT_URI functionality #3
Conversation
|
""" WalkthroughA new CMake module, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CMake
participant CMUTIL_NORMALIZE_GIT_URI
User->>CMake: Call CMUTIL_NORMALIZE_GIT_URI(URI, TARGET_TYPE, OUT_VAR)
CMake->>CMUTIL_NORMALIZE_GIT_URI: Parse arguments
alt TARGET_TYPE is SSH
CMUTIL_NORMALIZE_GIT_URI->>CMUTIL_NORMALIZE_GIT_URI: _CMUTIL_NORMALIZE_GIT_URI_SSH(URI, OUT_VAR)
else TARGET_TYPE is HTTP
CMUTIL_NORMALIZE_GIT_URI->>CMUTIL_NORMALIZE_GIT_URI: _CMUTIL_NORMALIZE_GIT_URI_HTTP(URI, OUT_VAR)
else Invalid TARGET_TYPE
CMUTIL_NORMALIZE_GIT_URI->>CMake: Fatal error
end
CMUTIL_NORMALIZE_GIT_URI->>CMake: Set OUT_VAR in parent scope
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Depends on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
FindCMUTIL.cmake (1)
21-21: Good addition – remember to keep includes alphabetisedThe extra include is correct; however, consider keeping the list alphabetically sorted for quick scanning (NORMALIZE comes after PLATFORM).
README.md (2)
10-10: Bullet order driftThe new entry is on top, breaking the pre-existing alphabetical order (N, P, T, V). Re-ordering keeps the list predictable.
25-30: Link table mis-alignmentColumn spacing of the reference links is now uneven; run a formatter or align by hand for readability.
test/NORMALIZE_GIT_URI/fail/invalid_http_uri/CMakeLists.txt (1)
10-16: Fail test could silently succeed if CMUTIL is missingAdd
REQUIREDto thefind_packageto guarantee the test actually fails on missing CMUTIL instead of passing:-FIND_PACKAGE(CMUTIL) +FIND_PACKAGE(CMUTIL REQUIRED)Minor, but avoids false-positive green builds when the module is not found.
test/NORMALIZE_GIT_URI/fail/CMakeLists.txt (1)
10-12: Regexes: tighten or anchor to avoid accidental matchesThe failure-expectation regexes are very generic; e.g.
"Invalid TARGET_TYPE"would match any message containing those words. Consider anchoring or using full error text to prevent false positives if unrelated diagnostics include the same fragment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
FindCMUTIL.cmake(1 hunks)README.md(2 hunks)system_modules/CMUTIL_NORMALIZE_GIT_URI.cmake(1 hunks)test/CMakeLists.txt(1 hunks)test/NORMALIZE_GIT_URI/CMakeLists.txt(1 hunks)test/NORMALIZE_GIT_URI/fail/CMakeLists.txt(1 hunks)test/NORMALIZE_GIT_URI/fail/invalid_http_uri/CMakeLists.txt(1 hunks)test/NORMALIZE_GIT_URI/fail/invalid_ssh_uri/CMakeLists.txt(1 hunks)test/NORMALIZE_GIT_URI/fail/invalid_target_type/CMakeLists.txt(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
test/NORMALIZE_GIT_URI/fail/invalid_http_uri/CMakeLists.txt
[error] 20-20: Unknown CMake command "TEST_VAR_EQUALS_LITERAL". Called from TEST_SSH_TARGET_TYPE at line 118 and TEST_RUN at line 12.
test/NORMALIZE_GIT_URI/fail/invalid_target_type/CMakeLists.txt
[error] 20-20: Unknown CMake command "TEST_VAR_EQUALS_LITERAL". Called from TEST_SSH_TARGET_TYPE at line 118 and TEST_RUN at line 12.
test/CMakeLists.txt
[error] 20-20: Unknown CMake command "TEST_VAR_EQUALS_LITERAL". Called from TEST_SSH_TARGET_TYPE at line 118 and TEST_RUN at line 12.
test/NORMALIZE_GIT_URI/fail/CMakeLists.txt
[error] 20-20: Unknown CMake command "TEST_VAR_EQUALS_LITERAL". Called from TEST_SSH_TARGET_TYPE at line 118 and TEST_RUN at line 12.
test/NORMALIZE_GIT_URI/fail/invalid_ssh_uri/CMakeLists.txt
[error] 20-20: Unknown CMake command "TEST_VAR_EQUALS_LITERAL". Called from TEST_SSH_TARGET_TYPE at line 118 and TEST_RUN at line 12.
test/NORMALIZE_GIT_URI/CMakeLists.txt
[error] 20-20: Unknown CMake command "TEST_VAR_EQUALS_LITERAL". Called from TEST_SSH_TARGET_TYPE at line 118 and TEST_RUN at line 12.
🔇 Additional comments (4)
test/NORMALIZE_GIT_URI/CMakeLists.txt (1)
118-121: Approve comprehensive test coverage.The test functions provide excellent coverage of different URI formats including custom domains, ports, subgroups, and repositories with/without
.gitextensions. The test structure is well-organized.system_modules/CMUTIL_NORMALIZE_GIT_URI.cmake (3)
1-15: Excellent documentation and module structure.The documentation clearly explains the purpose, formats, and functionality of the module. The examples are helpful and the structure is well-organized.
41-72: Well-structured main function with proper error handling.The main function correctly delegates to helper functions based on the target type and provides clear error messages for invalid target types.
120-124: Fix port handling in SSH URI conversion.The current implementation doesn't properly handle ports in HTTP URIs when converting to SSH format. URLs with ports like
https://host:8080/pathshould preserve the port information correctly.- STRING(REGEX REPLACE "^https?://([^/]+)/(.+)$" "\\1" hostname "${uri}") - STRING(REGEX REPLACE "^https?://([^/]+)/(.+)$" "\\2" path "${uri}") + STRING(REGEX REPLACE "^https?://([^/]+)(:[0-9]+)?/(.+)$" "\\1\\2" hostname "${uri}") + STRING(REGEX REPLACE "^https?://([^/]+)(:[0-9]+)?/(.+)$" "\\3" path "${uri}")Note: The resulting SSH URI format with ports (
git@host:port:path) is non-standard. Consider if this is the intended behavior.Likely an incorrect or invalid review comment.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
Chores