-
-
Notifications
You must be signed in to change notification settings - Fork 975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added source control push and pull functionality #1785
base: main
Are you sure you want to change the base?
Conversation
…here is not a tracked branch.
…trol and moved sheet views into it from the Source Control Navigator.
I am struggling to get the "Source Control" menu working here, specifically trying to gain access to the focused windows |
… we need to refresh git data right away rather than on navigator view init.
… and source control menu.
… and source control menu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just small changes
...ures/NavigatorArea/SourceControlNavigator/Changes/Views/SourceControlNavigatorSyncView.swift
Outdated
Show resolved
Hide resolved
...ures/NavigatorArea/SourceControlNavigator/Changes/Views/SourceControlNavigatorSyncView.swift
Outdated
Show resolved
Hide resolved
Looks good, but no tests are present |
@matthijseikelenboom We need a separate issue to add tests to not just what I did in this PR but everything source control related. |
command += " \(remote) \(branch)" | ||
} | ||
|
||
if rebase == true { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you checking if it's explicitly true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it might be either nil
or false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It never needs to be nil only false or true, i would opt for always returning false instead of nil
|
||
extension GitClient { | ||
/// Pull changes from remote | ||
func pullFromRemote(remote: String? = nil, branch: String? = nil, rebase: Bool? = nil) async throws { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are the params optional, these would almost always be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users can do a git pull
and it will pull their current branch without needing to specify. I am keeping close to how git works in this regard. Probably not extremely necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using just git pull is not very effective when it comes to big projects. I would perhaps rethink how this is managed. Because if something goes wrong with git users won't be too happy
func pullFromRemote(remote: String? = nil, branch: String? = nil, rebase: Bool? = nil) async throws { | ||
var command = "pull" | ||
|
||
if let remote = remote, let branch = branch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done better in my opinion:
func pullFromRemote(remote: String = "origin",
branch: String = "main",
rebase: Bool = false) async throws {
var command = "pull"
if remote == "" {
print("Remote needs to be specified")
return
}
if branch == "" {
print("Branch needs to specified")
return
}
command += " \(remote) \(branch)"
if rebase {
command += " --rebase"
}
_ = try await self.run(command)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are making the assumption that their default branch is "main".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are making the assumption that their default branch is "main".
Nope, just an example, because you would always need to pull the origin and branch regardless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'd want to default to nothing, in case of remote name and branch name.
The rebase value should indeed be non-nullable. But, since there is also the option to use --merge or --ff-only. might want to make it an inner private enum, instead of a boolean.
But if you'd follow clean code, you want four functions:
pullFromRemoteWithRebase
pullFromRemoteWithMerge
pullFromRemoteWithFastForward
pullFromRemoteWithGlobalConfig
Or something like that. Having a (boolean) flag is bad practice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest making that one function instead of multiple, you would end up with repetitive code if you make multiple functions
Description
Added push and pull sheet.
Renamed Features/Git to Features/SourceControl and moved sheet views into it from the Source Control Navigator.
Testing
Note to reviewers... Please pull down and test this in addition to reviewing the code. Once you have this pulled down...
In a terminal run...
Then add the remote in CodeEdit by
Checklist
Screenshots