fix: comments #35 #40
Conversation
|
🎉 All unit tests passed! ✅ |
There was a problem hiding this comment.
Pull request overview
This PR addresses feedback from PR #35 by refactoring naming conventions, improving documentation, and simplifying test code. The changes focus on making the codebase more maintainable with clearer naming and better inline documentation.
- Renamed
pull_requests_open()method tolist_open()for better API clarity - Renamed
Userstruct toPullRequestUserto avoid generic naming and potential conflicts - Enhanced documentation for
PullRequestItemsto clarify pagination behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/pull_requests.rs | Renamed method and struct for clarity, added documentation comments explaining pagination behavior |
| src/cli.rs | Updated function call to use renamed list_open() method |
| tests/network_tests.rs | Simplified test assertion logic by removing match statement in favor of direct assertions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/network_tests.rs
Outdated
| assert!(result.is_ok()); | ||
| let prs = result.unwrap(); | ||
| assert_eq!(prs.total_count, 2); | ||
| assert_eq!(prs.items.len(), 2); |
There was a problem hiding this comment.
Using assert!(result.is_ok()) followed by unwrap() is an anti-pattern. If result.is_ok() passes but unwrap() panics (which shouldn't happen but violates the DRY principle), you're testing the same condition twice. Consider using result.expect("Failed to get pull requests") or directly unwrap() instead, which will provide clear error messages if the test fails.
| assert!(result.is_ok()); | |
| let prs = result.unwrap(); | |
| assert_eq!(prs.total_count, 2); | |
| assert_eq!(prs.items.len(), 2); | |
| let prs = result.expect("Failed to get pull requests"); | |
| assert_eq!(prs.total_count, 2); | |
| assert_eq!(prs.items.len(), 2); | |
| assert_eq!(prs.items.len(), 2); |
src/pull_requests.rs
Outdated
| @@ -19,19 +20,20 @@ pub struct PullRequestResponse { | |||
| /// PR title | |||
| pub title: String, | |||
| /// User information who opened the PR | |||
There was a problem hiding this comment.
The comment should read "User information for the user who opened the PR" or "Information about the user who opened the PR" for better grammar and clarity.
| /// User information who opened the PR | |
| /// User information for the user who opened the PR |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
tests/network_tests.rs
Outdated
| let prs = result.expect("Failed to get pull requests"); | ||
| assert_eq!(prs.total_count, 2); | ||
| assert_eq!(prs.items.len(), 2); | ||
| assert_eq!(prs.items.len(), 2); |
There was a problem hiding this comment.
Duplicate assertion replaces actual test coverage
Line 68 is a duplicate of line 67 (assert_eq!(prs.items.len(), 2)), while the original assertions for prs.items[0].title and prs.items[0].user.login were removed. This loses test coverage for validating that PR titles and user logins are correctly parsed from the API response. The duplicate assertion provides no additional value and appears to be an accidental copy-paste error.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pull_requests.rs
Outdated
|
|
||
| impl PullRequests { | ||
| pub async fn pull_requests_open() -> Result<(), reqwest::Error> { | ||
| /// Fetch all open pull requests |
There was a problem hiding this comment.
The documentation comment says "Fetch all open pull requests" but based on the implementation and the updated documentation for PullRequestItems.items, this method only fetches the first page of results due to pagination. Consider updating the comment to reflect this limitation, e.g., "Fetch open pull requests (paginated results)".
| /// Fetch all open pull requests | |
| /// Fetch open pull requests (first page of paginated results) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
✨ Summary
I fixed copilot comments on #35 PR
🔧 Type of Change
Note
Refactor PR listing and models
PullRequests::pull_requests_opentoPullRequests::list_openand update CLI dispatch insrc/cli.rsto calllist_openUsertoPullRequestUserinsrc/pull_requests.rs; adjustPullRequestResponse.usertype and improve docsPullRequestItemswithitems(current page) andtotal_count(all pages);list_openprints totals and shows a tabletests/network_tests.rsto unwrap the network result and asserttotal_countanditems.len()Written by Cursor Bugbot for commit cbb5af4. This will update automatically on new commits. Configure here.