Fix GitLab Author Metadata: Use username Instead of author_name, …
#1186
Conversation
…Add `author_username` Trait, and Improve Release Metadata Extraction (#1)
|
Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️ |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances pull request metadata by introducing a unified author_username method across all supported remotes and updating release metadata to leverage it.
- Added
author_usernameto theRemotePullRequesttrait with a defaultNoneimplementation. - Implemented
author_usernamefor GitHub, GitLab, and Bitbucket pull request types. - Updated the release metadata macro to use
author_usernameand adjusted existing tests for compatibility.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/remote/mod.rs | Added trait method and updated macro to use it. |
| src/remote/github.rs | Introduced GitHubPullRequestUser and author_username. |
| src/remote/gitlab.rs | Implemented author_username for GitLabMergeRequest. |
| src/remote/bitbucket.rs | Implemented author_username for BitbucketPullRequest. |
| src/release.rs | Adjusted tests to include new user field. |
Comments suppressed due to low confidence (3)
git-cliff-core/src/remote/github.rs:100
- [nitpick] Consider renaming
GitHubPullRequestUsertoGitHubPullRequestAuthor(and theuserfield accordingly) to better reflect its role as the PR author and align with other platforms.
pub struct GitHubPullRequestUser {
git-cliff-core/src/remote/gitlab.rs:198
- There are no unit tests verifying that
author_usernamereturns the expected username for GitLab. Consider adding a test that constructs aGitLabMergeRequestwith a known author.username and asserts the method's output.
fn author_username(&self) -> Option<String> {
git-cliff-core/src/remote/bitbucket.rs:153
- Similarly, add a unit test for
BitbucketPullRequestto ensureauthor_usernamecorrectly returns theauthor.loginvalue.
fn author_username(&self) -> Option<String> {
| let username = pull_request | ||
| .and_then(|pr| pr.author_username()) | ||
| .or_else(|| v.username()); | ||
| commit.$remote.username = username.clone(); |
There was a problem hiding this comment.
[nitpick] The cloning of username could be streamlined by directly assigning the Option<String> rather than calling .clone() on the final value. For example, bind the result of .or_else(...) directly to the target field.
| commit.$remote.username = username.clone(); | |
| commit.$remote.username = username; |
Copilot uses AI. Check for mistakes.
|
@orhun Hi, I’ve proposed a fix, but this is only my third contribution to a Rust project, so I may need some guidance. I’m a DevOps Engineer rather than a developer, so I apologize in advance if anything is off. :) I’d really appreciate it if you could take a look—and feel free to correct or suggest improvements where needed. |
There was a problem hiding this comment.
Just to confirm, is this PR meant to address issue #687?
From my understanding, the root cause seems to be related to deserialization of the GitLab API response. If that's correct, it seems that the current changes might not fully resolve the issue — but please let me know if I misunderstood anything.
| @@ -354,7 +358,10 @@ macro_rules! update_release_metadata { | |||
| pr.merge_commit() == Some(v.id().clone()) || | |||
| pr.merge_commit() == sha_short | |||
| }); | |||
| commit.$remote.username = v.username(); | |||
| let username = pull_request | |||
| .and_then(|pr| pr.author_username()) | |||
There was a problem hiding this comment.
Since commit.remote.username is a field intended to represent the commit author, I believe it would be problematic to associate the PR author here if they are different. Linking the PR author as the commit author in this context might lead to confusion or misrepresentation.
There was a problem hiding this comment.
The main issue was that the username was incorrectly read as author_name, which resulted in the metadata being absent in most cases. I noticed this when I wondered why it wasn’t returning @username. I then called the API in the same way as git-cliff, and in that case, the information was correct. To confirm, I ran a test and compiled with these changes, and voilà—the metadata appeared! There may still be some room for fine-tuning, but the outcome was already much better than before. Therefore, I am confident that this is the root cause of the problem.
There was a problem hiding this comment.
This is definitely caused by this swap:
"gitlab": {
"username": null,
"pr_title": null,
"pr_number": null,
"pr_labels": [],
"is_first_time": false
},
There was a problem hiding this comment.
Linking the PR author as the commit author in this context might lead to confusion or misrepresentation.
Maybe you’re right! In that case, we can probably use the correct username from the commit—it should also be available there. The main problem is that author_name can be anything; it is not a unique identifier that can be easily used to match records.
There was a problem hiding this comment.
Thanks for the replay.
My original concern was more about the semantic meaning of the commit.remote.username field. Even if we now retrieve the username, if that value corresponds to the PR author and not the commit author, I believe it could still lead to incorrect attribution.
git-cliff/git-cliff-core/src/remote/mod.rs
Lines 94 to 95 in 2606366
|
I'm not sure how this is fixing #687 - can you take a step back and explain the actual problem behind the missing metadata and how this PR is fixing it? |
…Add
author_usernameTrait, and Improve Release Metadata Extraction (#1)Description
This pull request introduces support for retrieving the author's username from pull requests across different remote platforms (GitHub, GitLab, and Bitbucket). It also updates the release metadata logic to incorporate this new functionality. Below is a summary of the most important changes:
Enhancements to
RemotePullRequestTrait:author_usernameto theRemotePullRequesttrait, which returns the username of the pull request's author. A default implementation returningNoneis provided for backward compatibility. (git-cliff-core/src/remote/mod.rs, git-cliff-core/src/remote/mod.rsR118-R121)Platform-Specific Implementations:
GitHub:
GitHubPullRequestUserto represent the pull request author.GitHubPullRequeststruct to include auserfield of typeOption<GitHubPullRequestUser>.author_usernamemethod forGitHubPullRequestto return the author's username. (git-cliff-core/src/remote/github.rs, [1] [2] [3]GitLab: Implemented the
author_usernamemethod forGitLabMergeRequestto return the author's username. (git-cliff-core/src/remote/gitlab.rs, git-cliff-core/src/remote/gitlab.rsR197-R200)Bitbucket: Implemented the
author_usernamemethod forBitbucketPullRequestto return the author's username. (git-cliff-core/src/remote/bitbucket.rs, git-cliff-core/src/remote/bitbucket.rsR152-R155)Updates to Release Metadata Logic:
update_release_metadatamacro to use theauthor_usernamemethod when setting the username in release metadata. This ensures that the author's username is correctly captured from the associated pull request, falling back to the existing logic if unavailable. (git-cliff-core/src/remote/mod.rs, git-cliff-core/src/remote/mod.rsL357-R364)Test Adjustments:
git-cliff-core/src/release.rsto include theuserfield with aNonevalue for pull requests, ensuring compatibility with the newGitHubPullRequeststructure. (git-cliff-core/src/release.rs, [1] [2] [3] [4] [5]Motivation and Context
The primary motivation was to resolve the issue with loading metadata from GitLab.
#810, #687
How Has This Been Tested?
I’m not much of a programmer—especially not in Rust—but I was able to identify the root of the issue. The tests still need to be completed, and a proper code review is required. (There may be other things as well, but I’m probably not equipped to handle those.)
Screenshots / Logs (if applicable)
N/A
Types of Changes
Checklist: