Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Maintainer workflow #567
Maintainer workflow #567
Conversation
When PR clicked in PR list.
Added avatar and clickable PR# button that takes user to GitHub.
According to mockup by @donokuda. Data for the view is present in the designer view model but not yet in the real view model.
Open/Closed display not yet functional however, and the positioning of the "View more on GitHub" link is broken.
And provide a tooltip in case the source branch name doesn't fit in the pane.
And go straight to ApiClient. I don't know if we'll be caching this, but for now it makes things easier to hack on - we can re-add to the caching layer if needed later.
To display the files changed section. Also update sample data with change count.
WPF's ScrollViewer is broken, so hack around it. We already had this hack in GitHubConnectContent, so moved the hack to a central place that can be used wherever it's needed.
And fix bug in Account where avatar changes weren't getting notified.
It's ugly but it's there.
And removed a duplicate property.
I really want all caps for totally subjective aesthetical reasons, but it looks like we don't get that out of the box😭 https://stackoverflow.com/questions/1762485/how-to-make-all-text-upper-case-capital
| // TODO: Catch errors. | ||
| Observable.CombineLatest( | ||
| repositoryHost.ApiClient.GetPullRequest(repository.Owner, repository.CloneUrl.RepositoryName, prNumber), | ||
| repositoryHost.ApiClient.GetPullRequestFiles(repository.Owner, repository.CloneUrl.RepositoryName, prNumber).ToList(), |
shana
Oct 17, 2016
Collaborator
The ModelService is the one that should be loading the data from ApiClient, optionally caching it, and then building a IPullRequestModel with all the information that we need to show on the view model.
The ModelService is the one that should be loading the data from ApiClient, optionally caching it, and then building a IPullRequestModel with all the information that we need to show on the view model.
grokys
Oct 17, 2016
Author
Contributor
Yes, I did originally start writing it using ModelService but then I remembered how we ended up essentially bypassing the caching for the PR list, so I went straight to the API. The question I wanted to discuss with you was do our reasons for bypassing the cache for the PR list apply here?
Yes, I did originally start writing it using ModelService but then I remembered how we ended up essentially bypassing the caching for the PR list, so I went straight to the API. The question I wanted to discuss with you was do our reasons for bypassing the cache for the PR list apply here?
shana
Oct 17, 2016
Collaborator
In the case of the PR list, we're not bypassing the cache, we're aggressively refreshing. If the network goes down or the connection is slow, it can still load data from the cache while it tries to refresh. But regardless of whether we bypass the cache completely or partially, the model service's job is to take data and create models from it, caching is just an intermediate step that it can also do. We should not be using Octokit types anywhere if we can avoid it, we already have enough types to mock :P
In the case of the PR list, we're not bypassing the cache, we're aggressively refreshing. If the network goes down or the connection is slow, it can still load data from the cache while it tries to refresh. But regardless of whether we bypass the cache completely or partially, the model service's job is to take data and create models from it, caching is just an intermediate step that it can also do. We should not be using Octokit types anywhere if we can avoid it, we already have enough types to mock :P
grokys
Oct 17, 2016
Author
Contributor
Ok, that makes sense. I did assume that I would need to use it in the end, but for development it was easier just to bypass and not have to maintain yet another class and interface. Will make it use the model service.
Ok, that makes sense. I did assume that I would need to use it in the end, but for development it was easier just to bypass and not have to maintain yet another class and interface. Will make it use the model service.
|
|
||
| namespace GitHub.ViewModels | ||
| { | ||
| public class PullRequestFileViewModel : IPullRequestFileViewModel |
shana
Oct 17, 2016
Collaborator
Shouldn't this be a model and not a viewmodel?
Shouldn't this be a model and not a viewmodel?
grokys
Oct 17, 2016
Author
Contributor
It's specifically tailored to the needs of the view, so that makes it a view model to me. In particular it wouldn't be a tree if it weren't for the fact that we can display as a tree in the view and later we might want to add things like a custom icon depending on file type etc, which makes it a VM in my mind.
It's specifically tailored to the needs of the view, so that makes it a view model to me. In particular it wouldn't be a tree if it weren't for the fact that we can display as a tree in the view and later we might want to add things like a custom icon depending on file type etc, which makes it a VM in my mind.
shana
Oct 17, 2016
Collaborator
I don't see anything that's specific to this view in particular. It's just a representation of a file change in a pull request. It has no logic, only data, usable by any view or viewmodel to do stuff. Screams of model to me 😄
I don't see anything that's specific to this view in particular. It's just a representation of a file change in a pull request. It has no logic, only data, usable by any view or viewmodel to do stuff. Screams of model to me
grokys
Oct 17, 2016
Author
Contributor
Well to me the model is the model you get from the backend (PullRequestFile currently). This is transformed into a tree at the viewmodel layer for view purposes, making it a view model :)
Well to me the model is the model you get from the backend (PullRequestFile currently). This is transformed into a tree at the viewmodel layer for view purposes, making it a view model :)
|
|
||
| namespace GitHub.ViewModels | ||
| { | ||
| public class PullRequestDirectoryViewModel : IPullRequestDirectoryViewModel |
shana
Oct 17, 2016
Collaborator
Shouldn't this be a model?
Shouldn't this be a model?
shana
Oct 17, 2016
Collaborator
Actually, this is only used internally in the PullRequestDetailViewModel, so it probably shouldn't be exposed publicly, it's an implementation detail. We can just have this class in the PullRequestDetailViewModel as a helper class. I would definitely still not call it a ViewModel - the only thing it does is represent data, it has no logic for a view to interact with data, it's just a handy model (or in this case, a handy helper class)
Actually, this is only used internally in the PullRequestDetailViewModel, so it probably shouldn't be exposed publicly, it's an implementation detail. We can just have this class in the PullRequestDetailViewModel as a helper class. I would definitely still not call it a ViewModel - the only thing it does is represent data, it has no logic for a view to interact with data, it's just a handy model (or in this case, a handy helper class)
| Number = pullRequest.Number; | ||
| Author = new Models.Account(pullRequest.User, avatarProvider.GetAvatar(new AccountCacheItem(pullRequest.User))); | ||
| CreatedAt = pullRequest.CreatedAt; | ||
| Body = !string.IsNullOrWhiteSpace(pullRequest.Body) ? pullRequest.Body : "*No description provided.*"; |
shana
Oct 17, 2016
Collaborator
Instead of copying all of this to the view model, how about using the IPullRequestModel which is already set up for most of this information (adding any missing bits) and exposing a PullRequest property that the view can get the information from?
Instead of copying all of this to the view model, how about using the IPullRequestModel which is already set up for most of this information (adding any missing bits) and exposing a PullRequest property that the view can get the information from?
grokys
Oct 17, 2016
Author
Contributor
Yes, I did consider this but then we'd lose out on change notifications, as models shouldn't be implementing INotifyPropertyChanged. I could however use the IPullRequestModel fields as the backing instead of having them as fields in the VM itself.
Yes, I did consider this but then we'd lose out on change notifications, as models shouldn't be implementing INotifyPropertyChanged. I could however use the IPullRequestModel fields as the backing instead of having them as fields in the VM itself.
Not yet caching it yet though.
And bind to that where we don't need to do any transformation for the view.
- Hide Open/Diff menu for now - Use correct property for view switching menu item header - Display changed files count
Use Node rather than ViewModel.
|
Wow, this is really cool stuff. I found one serious problem with a When you fix that problem, I'd like to take another look. Let me know if there's any specific areas you'd like me to pay close attention to when reviewing. |
| get { return isOpen; } | ||
| set { isOpen = value; this.RaisePropertyChange(); } | ||
| get { return status; } | ||
| set { status = value; this.RaisePropertyChange(); this.RaisePropertyChange(nameof(IsOpen)); } |
haacked
Oct 20, 2016
Contributor
Maybe a comment here to explain that the second RaisePropertyChange event will go away when this is merged to master? Also, shouldn't this.RaisePropertyChange(nameof(Merged)) also be called here?
Maybe a comment here to explain that the second RaisePropertyChange event will go away when this is merged to master? Also, shouldn't this.RaisePropertyChange(nameof(Merged)) also be called here?
grokys
Oct 21, 2016
Author
Contributor
Good catch! Yes Merged should also be notified.
Good catch! Yes Merged should also be notified.
| public DateTimeOffset CreatedAt { get; set; } | ||
| public DateTimeOffset UpdatedAt { get; set; } | ||
| public IAccount Author { get; set; } | ||
| public IList<IPullRequestFileModel> ChangedFiles { get; set; } = new IPullRequestFileModel[0]; |
haacked
Oct 20, 2016
Contributor
In general, I'm wary of collection properties that can be read/write like this because it creates two ways of updating the collection, setting it and mutating it. It's probably a good idea to pick one approach or the other to avoid confusion. For example, you could make this an immutable IReadOnlyCollection<IPullRequestFileModel> and always set it when it changes.
Or you could make this a List<IPullRequestFileModel> (or some form of observable collection) and mutate the list when the collection changes (even if it means calling Clear and AddRange every time).
In general, I'm wary of collection properties that can be read/write like this because it creates two ways of updating the collection, setting it and mutating it. It's probably a good idea to pick one approach or the other to avoid confusion. For example, you could make this an immutable IReadOnlyCollection<IPullRequestFileModel> and always set it when it changes.
Or you could make this a List<IPullRequestFileModel> (or some form of observable collection) and mutate the list when the collection changes (even if it means calling Clear and AddRange every time).
grokys
Oct 21, 2016
Author
Contributor
Yes, again a very good point - I always forget about Dre IReadOnlyCollection.
Yes, again a very good point - I always forget about Dre IReadOnlyCollection.
| return new PullRequestCacheItem(pr, new PullRequestFile[0]); | ||
| } | ||
|
|
||
| public static PullRequestCacheItem Create(PullRequest pr, IList<PullRequestFile> files) |
haacked
Oct 20, 2016
Contributor
A nitpick here, but could this be IReadOnlyList<PullRequestFile>? I really like it when our parameters communicate more about what's needed and how they will be used. If you accept an IList I wonder if you're going to modify that list and whether I need to copy it first. Etc.
A nitpick here, but could this be IReadOnlyList<PullRequestFile>? I really like it when our parameters communicate more about what's needed and how they will be used. If you accept an IList I wonder if you're going to modify that list and whether I need to copy it first. Etc.
grokys
Oct 21, 2016
Author
Contributor
👍
| } | ||
| } | ||
|
|
||
| static string GetSafeBranchName(string name) |
haacked
Oct 20, 2016
Contributor
This implementation would replace / in a branch name, but that's totally valid. We have a good implementation of this in GitHub Desktop for Windows you can steal. I'll post the code here:
// See https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
// ASCII Control chars and space, DEL, ~ ^ : ? * [ \ are not valid.
// | " < and > are technically valid in a refname as far as Git is concerned, but not valid on Windows
// Also invalid: the magic sequence @{, consecutive dots, leading and trailing dot, ref ending in .lock
static readonly Regex invalidCharsRegex = new Regex(@"[\x00-\x20\x7F~^:?*\[\\|""<>]|@{|\.\.+|^\.|\.$|\.lock$", RegexOptions.ECMAScript);
/// <summary>
/// Given a ref name, returns a safe version with invalid characters replaced with dashes.
/// </summary>
/// <param name="refName"></param>
/// <returns></returns>
public static string GetSafeReferenceName(string refName)
{
Ensure.ArgumentNotNull(refName, "refName");
var groups = refName.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries)
.Select(x => invalidCharsRegex.Replace(x, "-"));
return String.Join("/", groups).TrimStart('-');
}
This implementation would replace / in a branch name, but that's totally valid. We have a good implementation of this in GitHub Desktop for Windows you can steal. I'll post the code here:
// See https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
// ASCII Control chars and space, DEL, ~ ^ : ? * [ \ are not valid.
// | " < and > are technically valid in a refname as far as Git is concerned, but not valid on Windows
// Also invalid: the magic sequence @{, consecutive dots, leading and trailing dot, ref ending in .lock
static readonly Regex invalidCharsRegex = new Regex(@"[\x00-\x20\x7F~^:?*\[\\|""<>]|@{|\.\.+|^\.|\.$|\.lock$", RegexOptions.ECMAScript);
/// <summary>
/// Given a ref name, returns a safe version with invalid characters replaced with dashes.
/// </summary>
/// <param name="refName"></param>
/// <returns></returns>
public static string GetSafeReferenceName(string refName)
{
Ensure.ArgumentNotNull(refName, "refName");
var groups = refName.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries)
.Select(x => invalidCharsRegex.Replace(x, "-"));
return String.Join("/", groups).TrimStart('-');
}
grokys
Oct 21, 2016
Author
Contributor
The branch name that is created here is based on the PR title, so I think that carrying that through into the branch name wouldn't be desired. For example, if I create a PR called "Fix foo/bar errors" then it doesn't make much sense for the PR to be called pr/123-fix foo/bar errors especially as Visual Studio will display it in the "Manage Branches" window as:
- pr
- 123-fix foo
- bar errors
The branch name that is created here is based on the PR title, so I think that carrying that through into the branch name wouldn't be desired. For example, if I create a PR called "Fix foo/bar errors" then it doesn't make much sense for the PR to be called pr/123-fix foo/bar errors especially as Visual Studio will display it in the "Manage Branches" window as:
- pr
- 123-fix foo
- bar errors
- 123-fix foo
haacked
Oct 21, 2016
Contributor
Ah, I see. Ok, carry on. Is it possible for a PR title to only contain invalid characters? Then what would be the branch name? Just a single -?
Ah, I see. Ok, carry on. Is it possible for a PR title to only contain invalid characters? Then what would be the branch name? Just a single -?
grokys
Oct 21, 2016
Author
Contributor
Hmm I guess someone could create a PR using only punctuation and no letters or digits, though I'm not sure how likely it would be! And yeah, in that case the branch name would be PR/123--.
Hmm I guess someone could create a PR using only punctuation and no letters or digits, though I'm not sure how likely it would be! And yeah, in that case the branch name would be PR/123--.
grokys
Oct 24, 2016
Author
Contributor
Thinking about this a little over the weekend, I think the current implementation could cause problems with non-Latin character sets. So I guess that if the title has no Latin characters we should fall back to simply pr/123.
Thinking about this a little over the weekend, I think the current implementation could cause problems with non-Latin character sets. So I guess that if the title has no Latin characters we should fall back to simply pr/123.
| @@ -95,5 +255,33 @@ public IObservable<string> GetPullRequestTemplate(ILocalRepositoryModel reposito | |||
| return ret; | |||
| } | |||
|
|
|||
| void EnsurePullRefSpecExists(IRepository repo) | |||
haacked
Oct 20, 2016
Contributor
This method could be static.
This method could be static.
|
|
||
| public bool IsPullRequestFromFork(ILocalRepositoryModel repository, IPullRequestModel pullRequest) | ||
| { | ||
| return pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl() != repository.CloneUrl.ToRepositoryUrl(); |
haacked
Oct 20, 2016
Contributor
So the Repository object returned by the API has a fork boolean you could test first. It's not on the IPullRequestModel instance, so you'd need to plum it through. But it'd ballow you to do this:
return pullRequest.IsFork && pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl() != repository.CloneUrl.ToRepositoryUrl();
That lets you short circuit the URL comparison if it's not a fork.
So the Repository object returned by the API has a fork boolean you could test first. It's not on the IPullRequestModel instance, so you'd need to plum it through. But it'd ballow you to do this:
return pullRequest.IsFork && pullRequest.Head.RepositoryCloneUrl.ToRepositoryUrl() != repository.CloneUrl.ToRepositoryUrl();
That lets you short circuit the URL comparison if it's not a fork.
grokys
Oct 21, 2016
Author
Contributor
Ah, I didn't think of looking there. So if I check that do I still need to compare the clone URLs?
Ah, I didn't think of looking there. So if I check that do I still need to compare the clone URLs?
haacked
Oct 21, 2016
Contributor
Ah, I didn't think of looking there. So if I check that do I still need to compare the clone URLs?
Yeah, I suppose you still do because it's possible that the PR target is the fork and not upstream.
Ah, I didn't think of looking there. So if I check that do I still need to compare the clone URLs?
Yeah, I suppose you still do because it's possible that the PR target is the fork and not upstream.
| return Observable.Defer(async () => | ||
| { | ||
| var repo = gitService.GetRepository(repository.LocalPath); | ||
| var branchName = GetLocalBranchesInternal(repository, repo, pullRequest).First(); |
haacked
Oct 20, 2016
Contributor
So we're absolutely sure GetLocalBranchesInternal will never return an empty collection? Maybe assert that in debug?
So we're absolutely sure GetLocalBranchesInternal will never return an empty collection? Maybe assert that in debug?
grokys
Oct 21, 2016
Author
Contributor
👍
| { | ||
| Title = pr.Title; | ||
| Number = pr.Number; | ||
| Base = new GitReferenceModel { Label = pr.Base.Label, Ref = pr.Base.Ref, RepositoryCloneUrl = pr.Base.Repository.CloneUrl }; | ||
| Head = new GitReferenceModel { Label = pr.Head.Label, Ref = pr.Head.Ref, RepositoryCloneUrl = pr.Head.Repository.CloneUrl }; |
haacked
Oct 20, 2016
Contributor
It's possible for the pr.Head.Repository property to be null causing a NullReferenceException here. For example, check out this PR: scientistproject/Scientist.net#20 After it was merged, the user deleted the repository.
It's possible for the pr.Head.Repository property to be null causing a NullReferenceException here. For example, check out this PR: scientistproject/Scientist.net#20 After it was merged, the user deleted the repository.
grokys
Oct 21, 2016
Author
Contributor
Very good catch - I didn't think of that! Added code to deal with that case and a unit test. In addition, I've made GitReferenceModel immutable and made it check for non-null values in its fields as an additional sanity check.
Very good catch - I didn't think of that! Added code to deal with that case and a unit test. In addition, I've made GitReferenceModel immutable and made it check for non-null values in its fields as an additional sanity check.
| } | ||
|
|
||
| public string FileName { get; } | ||
| public string Path { get; } |
haacked
Oct 20, 2016
Contributor
I tend to find property names like Path confusing. Path to what? How about DirectoryName or DirectoryPath? Bonus, that'll let you use Path.GetFileName rather than the fully qualified calls. 😄
I tend to find property names like Path confusing. Path to what? How about DirectoryName or DirectoryPath? Bonus, that'll let you use Path.GetFileName rather than the fully qualified calls.
grokys
Oct 21, 2016
Author
Contributor
Thing is that this is either a path to a file or a directory, as the property comes from the IPullRequestChangeNode which is implemented by PullRequestDirectoryNode as well as PullRequestFileNode. As such, I'm struggling to come up with a better name.
Thing is that this is either a path to a file or a directory, as the property comes from the IPullRequestChangeNode which is implemented by PullRequestDirectoryNode as well as PullRequestFileNode. As such, I'm struggling to come up with a better name.
haacked
Oct 21, 2016
Contributor
Ah! I didn't catch that. Carry on.
Ah! I didn't catch that. Carry on.
And added comment indicating that it will be🔥 soon.
To try and minimize the chances of null exceptions.
If `PullRequestService.SwitchToBranch` is called with no local branch, handle it but assert.
When `CheckoutMode == Push` and has uncommitted changes.
This adds a PR details view to the GitHub pane, and allows checking out PR branches both from the same repository and from forks.
Known issues:
cc @haacked to notify you of the known issues above