-
Notifications
You must be signed in to change notification settings - Fork 112
Improve typing and documentation for repository URL parsing utility #215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughUpdated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/app/services/codegraph/repo_service.py (1)
75-79: Clear error message with helpful examples.The error message provides actionable guidance with the two most common input formats. Optionally, you could include all four formats mentioned in the docstring (lines 40-44) for complete consistency, but the current message is sufficient for most use cases.
💡 Optional: Add all supported formats to error message
raise ValueError( f"Invalid repository format: '{repo_input}'. " - "Expected formats include 'owner/repo' or " - "'https://github.com/owner/repo'." + "Expected formats: 'owner/repo', 'https://github.com/owner/repo', " + "'github.com/owner/repo', or URLs ending with '.git'." )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/services/codegraph/repo_service.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/services/codegraph/repo_service.py (1)
backend/app/database/supabase/client.py (1)
get_supabase_client(9-13)
🔇 Additional comments (3)
backend/app/services/codegraph/repo_service.py (3)
4-4: LGTM! Good addition of TypedDict import.The import is necessary for the new
ParsedRepoTypedDict and maintains compatibility with existing code that usesDictandAny.
12-23: Excellent type safety improvement!The
ParsedRepoTypedDict provides clear structure and type safety for repository parsing results. The documentation is comprehensive and accurately describes each field.
36-57: Outstanding documentation and type safety improvements!The enhanced return type and comprehensive docstring significantly improve code maintainability. The documentation clearly describes all supported input formats, return structure, and error conditions, making the function's behavior explicit for future developers.
|
@smokeyScraper check this out. This is a non-behavioural but necessary change to improve parsing clarity. |
Aditya30ag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add to much docstring as the variable name itself suggest it work.
- Introducing a TypedDict here doesn’t provide much benefit unless we’re enforcing static typing across the codebase.
- The original
dict[str, str]return type was sufficient and clearer.
|
Hey @Aditya30ag, Thanks for the clarification — that makes sense. |
Summary
This PR improves the clarity of the
_parse_repo_urlutility by introducingan explicit return type (
TypedDict) and enhancing documentation.Changes
ParsedRepoTypedDict to define the return schemaNotes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.