Repository model refactoring (part 3) #2028
Conversation
Refactor the constructor that took IGitService into LocalRepositoryModel where it was used.
Move GenerateUrl from LocalRepositoryModel to LinkCommandBase.
Move LocalRepositoryModel construction tests to new home.
This property wasn't being used.
We Now only need the CreateLocalRepositoryModel(localPath) overload.
Previously the current branch was being read when CurrentBranch was fetched. This changes it to be read when the LocalRepositoryModel is created.
Remove redundant code and usings.
Previously CurrentBranch was created as the property was read. We now need a way to refresh it.
It appears VSGitExt.ActiveRepositoriesChanged is fired before the local repository has actually changed its branch. This means we can't read the branch information immediately!
Remove the ILocalRepositoryModel.CurrentBranch property and explicitly call IGitService.CreateCurrentBranchModel instead. Fix all the broken tests.
Convert GetPullRequestForCurrentBranch to return (string owner, int number). This allows the tuple to be compared directly.
We can now use GitService as a LocalRepositoryMode factory.
Be explicit about what it does.
Make ITeamExplorerServiceHolder responsible for holding references to services, but not watching for and marshaling repository change events. Delegate to ITeamExplorerContext for this.
This is no longer called by TeamExplorerServiceHolder.
Contains a call chain that results in a call to a virtual method defined by the class.
Make log message a bit less bit yoda-y.
Tests started throwing null reference exceptions when we stopped using an interface for BranchModel. This makes PrepareTestData default to return no branches.
Remove Octokit.Repository dependency from RemoteRepositoryModel.
Fix warnings in LocalRepositoryModelDesigner and RemoteRepositoryModelDesigner.
jcansdale
changed the title
There was a problem hiding this comment.
Look good in general, and pretty straightfoward. However I have one question (and couldn't leave it inline as the file was moved without changes):
Why was Account moved into GitHub.Exports.Reactive? It's a class with logic so should really stay in GitHub.App I think.
| @@ -82,8 +82,12 @@ public class PullRequestCreationViewModel : PanePageViewModelBase, IPullRequestC | |||
| .Where(_ => TargetBranch != null) | |||
| .Subscribe(x => | |||
| { | |||
| //// HACK: Why is `t` null? | |||
| //if (!x.Any(t => t != null && t.Equals(TargetBranch))) | |||
There was a problem hiding this comment.
Does this comment mean "Why was there a check for t being null here?"
There was a problem hiding this comment.
I temporarily added the t != null to allow the tests to pass. For some reason changing from using an interface to a class meant this come up null when testing.
I believe it was because NSubstitute automatically generates substitutes rather than returning null by default. I've since fixed this in the tests and removed the null check/comment.
There was a problem hiding this comment.
Could we remove the commented-out code here then, or is that fix in a separate PR?
There was a problem hiding this comment.
Oops, in my mind I'd removed the comments! ;-)
jcansdale
changed the base branch from
refactor/ITeamExplorerServiceHolder
to
master
This no longer needs to be in GitHub.Exports.Reactive.
After some later cleanup, the constructor of |
Team Explorer was being flooded with repository StatusChanged events which were being processes asynchronously.
This appears to be redundant and not used.
| @@ -82,8 +82,12 @@ public class PullRequestCreationViewModel : PanePageViewModelBase, IPullRequestC | |||
| .Where(_ => TargetBranch != null) | |||
| .Subscribe(x => | |||
| { | |||
| //// HACK: Why is `t` null? | |||
| //if (!x.Any(t => t != null && t.Equals(TargetBranch))) | |||
There was a problem hiding this comment.
Could we remove the commented-out code here then, or is that fix in a separate PR?
Previously visibility was only being refreshed when a new repository change event arrived.
Doesn't belong in this PR!
Good catch! This has been fixed in 17716ef. |
Previously both TeamExplorerNavigationItemBase and TeamExplorerItemBase were listring for repository change events. This changes it so that only TeamExplorerItemBase needs to listen for change events. TeamExplorerNavigationItemBase needs to subscribe before the item is visible (the event is required to make it visible).
Remove redundant Guard statements.
Depends on #2008
Depends on #2025
The focus of this PR has been removing the model interfaces,
Octokitdependencies in model constructors and fixing NSubstitute tests that were broken when an interface changed to a class. I've tried to avoid any functional changes.What this PR does
IGitServicedependency fromBranchModelOctokit.Branchdependency fromBranchModelOctokit.Repositorydependency fromRemoteRepositoryModelIBranchinterface fromBranchModelIRepositoryModelinterface fromRepositoryModelIRemoteRepositoryModelinterface fromRemoteRepositoryModelILocalRepositoryModelinterface fromLocalRepositoryModelRemoteRepositoryModelfromGitHub.ApptoGitHub.Exports.ReactiveMovedAccountfromGitHub.ApptoGitHub.Exports.ReactiveIGitService.CreateCurrentBranchModeltoGetBranchBranchModelwhenLibGit2Sharp.Branch.IsRemote=trueStill to do
BranchModelto make it an actual model(@grokys)
Refactor repository models (repository refactor part 1) #2008 (comment)
BranchModelis being passed a repository constructed from a URL rather then a local pathVisualStudio/src/GitHub.Exports/Models/BranchModel.cs
Line 36 in e3f8114