Fix GitHub URL parsing for plural segments#38
Conversation
Reviewer's GuideThe PR introduces plural/singular segment support in parse_reference by deriving PartialEq on ResourceType, replacing the simple segment == type check with a composite matches condition (covering “pulls” and “issue”), updating the WrongResourceType error to report the expected segment variable, and adds tests for both plural and singular URL variants. Class diagram for updated ResourceType and parse_reference logicclassDiagram
class ResourceType {
<<enum>>
Issues
PullRequest
as_str() str
}
class RepoInfo {
owner: String
name: String
}
class VkError {
InvalidRef
WrongResourceType
}
class parse_reference {
+parse_reference(url: &str, resource_type: ResourceType) Result<(RepoInfo, u64), VkError>
}
RepoInfo <.. parse_reference
ResourceType <.. parse_reference
VkError <.. parse_reference
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughExtend the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant parse_reference
participant ResourceType
Caller->>parse_reference: Call with input URL, default_repo, resource_type
parse_reference->>parse_reference: Split URL into segments
parse_reference->>ResourceType: Check if segment matches resource_type or allowed variant
alt Segment matches
parse_reference->>parse_reference: Parse repo info and number
parse_reference-->>Caller: Return (RepoInfo, number)
else Segment does not match
parse_reference-->>Caller: Return WrongResourceType error
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs⚙️ CodeRabbit Configuration File
Files:
🔇 Additional comments (7)
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider centralizing valid URL segments for each ResourceType (singular and plural) in a single method to simplify the matching logic.
- The WrongResourceType error only reports the singular expected segment; consider updating it to list all accepted variants (issue and issues).
- Add a test for the plural issues URL path (e.g. /owner/repo/issues/123) to ensure symmetric coverage for issue URLs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing valid URL segments for each ResourceType (singular and plural) in a single method to simplify the matching logic.
- The WrongResourceType error only reports the singular expected segment; consider updating it to list all accepted variants (issue and issues).
- Add a test for the plural issues URL path (e.g. /owner/repo/issues/123) to ensure symmetric coverage for issue URLs.
## Individual Comments
### Comment 1
<location> `src/main.rs:872` </location>
<code_context>
- if segments.get(2).expect("length checked") == &resource_type.as_str() {
+ let segment = segments.get(2).expect("length checked");
+ let expected = resource_type.as_str();
+ let matches = segment == &expected
+ || (resource_type == ResourceType::PullRequest && *segment == "pulls")
+ || (resource_type == ResourceType::Issues && *segment == "issue");
</code_context>
<issue_to_address>
Consider moving the segment matching logic into the ResourceType enum and using a contains check in the parser for clarity and maintainability.
You can push all of that OR-logic back into `ResourceType` and make the parser just do one `.contains()` check:
```rust
#[derive(Clone, Copy, PartialEq)]
enum ResourceType {
Issues,
PullRequest,
}
impl ResourceType {
// co-locate both singular+plural variants here
fn allowed_segments(self) -> &'static [&'static str] {
match self {
ResourceType::Issues => &["issues", "issue"],
ResourceType::PullRequest => &["pull", "pulls"],
}
}
// keep this pointing at the primary (plural) form if you like
fn as_str(self) -> &'static str {
self.allowed_segments()[0]
}
}
```
Then your parser becomes:
```rust
let segment = segments.get(2).unwrap();
let found = *segment;
let allowed = resource_type.allowed_segments();
if allowed.contains(&found) {
// …same extraction logic…
return Ok((RepoInfo { owner, name }, number));
}
return Err(VkError::WrongResourceType {
expected: resource_type.as_str().to_owned(),
found: found.to_owned(),
});
```
This:
- Removes the inline OR‐chain
- Keeps all plural/singular mapping next to the enum
- Makes intent (`allowed.contains`) crystal-clear without changing behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai have the following review comments been fully addressed? Please address the comments from this code review: Overall Comments
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll examine the current code to check if those review comments have been addressed. 🏁 Script executed: Length of output: 1185 Let me get a better view of the current implementation: 🏁 Script executed: Length of output: 2576 Let me examine the allowed_segments method and the test cases: 🏁 Script executed: Length of output: 1440 Let me look at the test section to see what tests exist: 🏁 Script executed: Length of output: 5417 Let me search specifically for issue-related tests: 🏁 Script executed: Length of output: 4099 Let me get the complete issue-related test functions: 🏁 Script executed: Length of output: 372 Let me search more broadly for the parse_issue_reference calls: 🏁 Script executed: Length of output: 5050 @leynos Yes, all three review comments have been fully addressed: ✅ 1. Centralized valid URL segmentsThe code now has a centralized impl ResourceType {
fn allowed_segments(self) -> &'static [&'static str] {
match self {
Self::Issues => &["issues", "issue"],
Self::PullRequest => &["pull", "pulls"],
}
}
}This replaces the previous inline matching logic with a clean, maintainable approach. ✅ 2. WrongResourceType error shows all accepted variantsThe error now uses the centralized method: return Err(VkError::WrongResourceType {
expected: allowed, // This comes from resource_type.allowed_segments()
found: (*segment).to_owned(),
});This means the error will display all accepted variants (e.g., both "issue" and "issues") instead of just the singular form. ✅ 3. Test coverage for plural issues URLThere are comprehensive tests for issue URLs, including:
The implementation actually went beyond the requested test coverage, providing symmetric testing for both singular and plural forms for issues, just like what exists for pull requests. All review comments have been comprehensively addressed! 🎯 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
pullsandissuesegments inparse_referencecloses #19
Testing
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_68871bb7b41c8322a143a854b2bf8f52
Summary by Sourcery
Fix GitHub URL parsing to support both plural and singular path segments for issues and pull requests
Bug Fixes:
Enhancements:
Tests: