Refactor main into parser, diff, and query modules#57
Conversation
Reviewer's GuideThe PR refactors the monolithic main.rs by splitting out diff hunk formatting, GraphQL query constants, and reference parsing into dedicated modules, updating imports across the codebase and relocating associated tests. Class diagram for refactored reference parsing (ref_parser.rs)classDiagram
class RepoInfo {
+String owner
+String name
}
class ResourceType {
<<enum>>
Issues
PullRequest
+allowed_segments() &'static [&'static str]
}
class VkError
class ref_parser {
+parse_issue_reference(input: &str, default_repo: Option<&str>) Result<(RepoInfo, u64), VkError>
+parse_pr_reference(input: &str, default_repo: Option<&str>) Result<(RepoInfo, u64), VkError>
parse_reference(input: &str, default_repo: Option<&str>, resource_type: ResourceType) Result<(RepoInfo, u64), VkError>
repo_from_fetch_head() Option<RepoInfo>
repo_from_str(repo: &str) Option<RepoInfo>
}
ref_parser ..> RepoInfo
ref_parser ..> ResourceType
ref_parser ..> VkError
Class diagram for diff hunk formatting (diff.rs)classDiagram
class ReviewComment {
+String body
+String diff_hunk
+Option<i32> original_position
+Option<i32> position
+String path
+String url
+Option<Author> author
}
class diff {
+format_comment_diff(comment: &ReviewComment) Result<String, std::fmt::Error>
GUTTER_WIDTH: usize
}
diff ..> ReviewComment
Class diagram for GraphQL query constants (graphql_queries.rs)classDiagram
class graphql_queries {
+THREADS_QUERY: &str
+COMMENT_QUERY: &str
+ISSUE_QUERY: &str
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughSplit the parsing and formatting logic for GitHub references and diff hunks into dedicated modules. Move GraphQL query strings to a new module. Update imports and module declarations to reflect this modularisation. Refactor code to delegate reference parsing and diff formatting to the new modules, removing redundant logic from the main file. Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant RefParser
participant Diff
participant GraphQLQueries
Main->>RefParser: parse_issue_reference / parse_pr_reference(input, default_repo)
RefParser-->>Main: (RepoInfo, number) or error
Main->>GraphQLQueries: Use THREADS_QUERY / COMMENT_QUERY / ISSUE_QUERY
Main->>Diff: format_comment_diff(comment)
Diff-->>Main: formatted diff string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
✨ 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. 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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/ref_parser.rs:38` </location>
<code_context>
- clippy::result_large_err,
- reason = "VkError has many variants but they are small"
-)]
-fn parse_reference(
- input: &str,
- default_repo: Option<&str>,
</code_context>
<issue_to_address>
Consider refactoring parse_reference by extracting URL and repo parsing into helper functions to flatten control flow and centralize repeated logic.
Here’s one way to peel off the nested logic in `parse_reference` into focused helpers—this preserves all existing behavior but drastically flattens the main function and centralizes your “.git”‐suffix stripping and regex usage:
```rust
fn strip_git_suffix(name: &str) -> &str {
name.strip_suffix(".git").unwrap_or(name)
}
fn parse_github_url(
input: &str,
resource: ResourceType,
) -> Option<Result<(RepoInfo, u64), VkError>> {
// Try URL parsing
let url = Url::parse(input).ok()?;
if url.host_str()? != "github.com" {
return None;
}
let parts: Vec<_> = url.path_segments()?.collect();
if parts.len() < 4 {
return Some(Err(VkError::InvalidRef));
}
let owner = parts[0];
let repo = strip_git_suffix(parts[1]);
let seg2 = parts[2];
if !resource.allowed_segments().contains(&seg2) {
return Some(Err(VkError::WrongResourceType {
expected: resource.allowed_segments(),
found: seg2.to_owned(),
}));
}
let number = parts[3].parse().map_err(|_| VkError::InvalidRef)?;
Some(Ok((RepoInfo {
owner: owner.into(),
name: repo.into(),
}, number)))
}
fn parse_repo_str(repo: &str) -> Option<RepoInfo> {
if let Some(caps) = GITHUB_RE.captures(repo) {
let owner = caps.name("owner")?.as_str().to_owned();
let name = strip_git_suffix(caps.name("repo")?.as_str()).to_owned();
return Some(RepoInfo { owner, name });
}
// fallback to plain "foo/bar"
let mut parts = repo.splitn(2, '/');
let (o, n) = (parts.next()?, parts.next()?);
Some(RepoInfo {
owner: o.into(),
name: strip_git_suffix(n).into(),
})
}
fn repo_from_fetch_head() -> Option<RepoInfo> {
let content = fs::read_to_string(".git/FETCH_HEAD").ok()?;
content
.lines()
.filter_map(|l| parse_repo_str(l))
.next()
}
```
Then your `parse_reference` collapses to:
```rust
fn parse_reference(
input: &str,
default_repo: Option<&str>,
resource: ResourceType,
) -> Result<(RepoInfo, u64), VkError> {
// 1) URL case
if let Some(res) = parse_github_url(input, resource) {
return res;
}
// 2) Number + default‐repo case
if let Ok(num) = input.parse::<u64>() {
let repo = default_repo
.and_then(parse_repo_str)
.or_else(repo_from_fetch_head)
.ok_or(VkError::RepoNotFound)?;
return Ok((repo, num));
}
// 3) Anything else
Err(VkError::InvalidRef)
}
```
This
- removes inline “.git” stripping everywhere
- collapses both regex paths into a single `parse_repo_str`
- flattens the big `if … else if … else` tree into three very simple steps.
</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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/main.rs (1)
1-757: File exceeds 400 line limit.At 757 lines, this file violates the 400-line maximum. Further decompose the module by extracting:
- Thread and review fetching logic (lines 186-279)
- Summary generation and printing (lines 288-322)
- Configuration loading logic (lines 419-438)
- Test utilities and fixtures (lines 478-486, 568-574)
♻️ Duplicate comments (1)
src/ref_parser.rs (1)
38-80: Refactor parse_reference to reduce cyclomatic complexity.The function has high cyclomatic complexity with deeply nested control flow. Extract URL parsing, repo parsing, and validation logic into focused helper functions as suggested in the previous review.
The previous review comment provides an excellent refactoring approach that would:
- Reduce cyclomatic complexity from ~8 to ~3
- Centralise
.gitsuffix stripping logic- Improve readability and maintainability
- Separate URL parsing from number parsing concerns
Implement the suggested helper functions:
strip_git_suffix,parse_github_url,parse_repo_str, andrepo_from_fetch_head.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/diff.rs(1 hunks)src/graphql_queries.rs(1 hunks)src/main.rs(3 hunks)src/printer/mod.rs(2 hunks)src/ref_parser.rs(1 hunks)src/reviews.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/reviews.rssrc/printer/mod.rssrc/diff.rssrc/main.rssrc/graphql_queries.rssrc/ref_parser.rs
🔇 Additional comments (7)
src/reviews.rs (1)
10-10: Import path correctly updated for the refactored module structure.The change properly reflects the new location of
RepoInfoin theref_parsermodule.src/printer/mod.rs (1)
8-8: Import and usage correctly updated for the refactored diff module.The explicit import and direct function call improve code clarity.
Also applies to: 79-79
src/graphql_queries.rs (1)
1-61: Well-structured module for GraphQL queries.The module properly encapsulates all GraphQL query strings with clear naming and appropriate visibility. The module doc comment clearly describes its purpose.
src/diff.rs (1)
88-95: Unused parsed values are correctly prefixed.The
_old_countand_new_countvariables are parsed but unused. The underscore prefix correctly indicates this is intentional.src/main.rs (1)
10-14: Module imports correctly reflect the refactored structure.The new module declarations and imports properly integrate the extracted functionality.
Also applies to: 18-20
src/ref_parser.rs (2)
1-1: LGTM: Module documentation follows guidelines.The module doc comment properly explains the module's purpose using the required
//!format and follows en-GB spelling conventions.
139-249: LGTM: Comprehensive test coverage.The test suite covers all major parsing scenarios including URL parsing, git suffix handling, plural segments, and error cases. Tests follow good practices with descriptive names and clear assertions.
|
@coderabbitai please create a new GitHub issue for the following:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help you create a GitHub issue for the VkError optimization. Let me first search for the VkError definition and the specific FIXME comment to provide proper context. 🏁 Script executed: Length of output: 524 Let me get the full VkError enum definition to understand all the variants: 🏁 Script executed: Length of output: 614 Let me get the complete enum definition and look for any clippy warnings: 🏁 Script executed: Length of output: 4730 Now I understand! I can see there are multiple 🏁 Script executed: Length of output: 1650 @leynos: I've successfully created GitHub issue #58 for optimizing the VkError enum by boxing large variants. The issue includes:
The issue has been assigned to you and is available at: #58 |
1 participant
Summary
diffmodulegraphql_queriesref_parserand update importsTesting
make lintmake testcargo checkhttps://chatgpt.com/codex/tasks/task_e_6893db4d09b883228dd3df1e195c6c06
Summary by Sourcery
Refactor main.rs into dedicated modules for diff formatting, GraphQL queries, and reference parsing to improve code organization.
Enhancements: