Generate Copy/Open link based on the latest commit #553
Conversation
| return null; | ||
|
|
||
| var sha = HeadSha; | ||
| var sha = await GitService.GitServiceHelper.GetLatestPushedSha(path); |
There was a problem hiding this comment.
Is this the path where the repository is? The GenerateUrl method takes a path argument that represents the file to generate the url for, not the path to the repository where the file is.
|
|
||
| public static IGitService GitServiceHelper => VisualStudio.Services.DefaultExportProvider.GetExportedValueOrDefault<IGitService>() ?? new GitService(); | ||
|
|
||
| public Task<string> GetLatestPushedSha(string path) |
There was a problem hiding this comment.
Is this argument allowed to be null? If yes, it needs to be documented as such, and code that relies on it needs null checks. If no, it needs a null check at the start of the method.
shana
left a comment
shana
left a comment
There was a problem hiding this comment.
Items to fix before merging:
pathneeds to be checked for nullness inGetLatestPushedSha- Document method
| /// Finds the latest pushed commit of a file and returns the sha of that commit. Returns null when no commits have | ||
| /// been found in any remote branches or the current local branch. | ||
| /// </summary> | ||
| /// <param name="path">The local path of the file. This cannot be null.</param> |
There was a problem hiding this comment.
I think The local path of a repository or a file inside a repository. This cannot be null. will be clearer.
| public Task<string> GetLatestPushedSha(string path) | ||
| { | ||
| Guard.ArgumentNotNull(path, nameof(path)); | ||
| var repo = GetRepository(path); |
There was a problem hiding this comment.
If the path points to a file or a directory that's not inside a repository, this will return null, so you need to check that before using the variable, and likely return early.
| /// <returns></returns> | ||
| UriString GetRemoteUri(IRepository repo, string remote = "origin"); | ||
|
|
||
| Task<string> GetLatestPushedSha(string path); |
There was a problem hiding this comment.
You'll need to copy the documentation added to the method to here. Classes and interfaces don't share docs, unfortunately.
paladique
force-pushed
the
fixes/343-latest-remote-commit
branch
from
be53ecc to
1b8e445
Compare
|
✨ |
Fixes #343
Continuation of #510