Enable commit links for GitLab #972
Conversation
|
@dandavison, do let me know if there's anything I should fix / adjust on this one, or if you want an issue created related to the changes, etc -- happy to do the legwork, as long as I know what it is |
| self.commit_link_format | ||
| .replace("{repo_slug}", &self.repo_slug) | ||
| .replace("{commit}", commit) | ||
| } |
There was a problem hiding this comment.
Hi @jwarlander, thanks very much for this PR!
Sorry for being slow. I'd been caught between wanting to leave a comment to keep things moving and wanting to find time not to leave a lazy comment, if you know what I mean.
My one question about this is whether perhaps the implementation isn't quite an idiomatic way to do this in Rust.
Since a repo cannot simultaneously be a GitHub repo and a GitLab repo, I was thinking that the way we'd implement this would be to add a GitLabRepo variant to the GitRemoteRepo enum. Maybe you've already thought about this while you were implementing this PR. What do you think, could it maybe be something like the code fragment below?
#[derive(Clone, Debug, PartialEq)]
pub enum GitRemoteRepo {
GitHubRepo(String),
GitLabRepo(String),
}
impl GitRemoteRepo {
pub fn format_commit_url(&self, commit: &str) -> String {
match self {
Self::GitHubRepo(repo_slug) => {
format!("https://github.com/{}/commit/{}", repo_slug, commit)
}
Self::GitLabRepo(repo_slug) => {
format!("https://gitlab.com/{}/-/commit/{}", repo_slug, commit)
}
}
}
}
fn format_commit_line_captures_with_osc8_commit_hyperlink(
captures: &Captures,
repo: &GitRemoteRepo,
) -> String {
let commit = captures.get(2).unwrap().as_str();
format!(
"{prefix}{osc}8;;{url}{st}{commit}{osc}8;;{st}{suffix}",
url = repo.format_commit_url(commit),
commit = commit,
prefix = captures.get(1).map(|m| m.as_str()).unwrap_or(""),
suffix = captures.get(3).unwrap().as_str(),
osc = "\x1b]",
st = "\x1b\\"
)
}There might be better ways to do this; for example it would be nice if GitHubRepo(String) were more self-documenting regarding what the string data is.
There was a problem hiding this comment.
No worries, I definitely know how it goes, and I imagine you've got plenty to do besides working on this project 😅
Anyhow, yeah, that works of course!
It just didn't occur to me at the time of writing that you could actually have an impl for an enum type -- I've probably seen it while reading about enums, but I haven't had the opportunity to work with those a lot yet, and they're significantly more capable in Rust than other languages I've used 😎
There was a problem hiding this comment.
It would definitely be more self-documenting with the variants as structs, even if slightly more verbose; have a look at my most recent update to see how it would look.
|
Excellent, that looks perfect and I agree with you -- the structs are better. I've tested quickly on the iTerm2 repo and hyperlinks in git log and git blame link to the gitlab remote correctly. I'll merge this when the tests pass. Thanks a lot! |
| groups = caps.get(2).map(|x| x.as_str()).unwrap_or_default(), | ||
| repo = caps.get(3).unwrap().as_str() | ||
| ), | ||
| }) |
There was a problem hiding this comment.
If you felt like it, it might be nice to separate these into separate functions and have from_str implemented as "return the result from the first parser function that succeeds".
There was a problem hiding this comment.
My first thought, before the weekend, was to see if it could be done using serde.. But that seemed like total overkill 😅
It would be nice with something else in the long run.. Like, each repo type just knowing how its URL is represented, then registering itself so that the parser can try them all.
That could enable specifying full-fledged new repo types in a config file, say, for those who have hosted GitHub or GitLab instances, etc.
|
Merged. Thanks a lot for this work @jwarlander! |
This change makes sure that
deltarecognizes GitLab remotes, and renders the proper commit links without the user having to do custom configuration.