-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Simplify GitCommand #11233
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
Simplify GitCommand #11233
Conversation
Reset | ||
} | ||
|
||
public static partial class GitCommandFactory |
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 is not a factory in the "classical" sense - there'd be an interface (e.g., IGitCommandFactory
) and factory method (e.g., Create<TGitCommand>(params object[] args)
). But that'd mean an extra layer of abstraction and perf costs (e.g., boxing of valuetypes).
Also, the callsites look kind of awkward too, e.g., GitCommandFactory.CheckoutBranch(....)
.
The fact that this type and its members are static
isn't great either (this may affect testability), but we can revisit this later, if required.
I suggest renaming this to simply GitCommands
and have all the implementations in the same file as it facilitates the discoverability of the type when browsing the repo and the maintenance.
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 have renamed it to GitCmd
because we already have a namespace GitCommands
. It did not compile without specifying the parent namespace: Commands.GitCommands.CheckoutBranch(...)
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.
Boo... I kind of missed that.
Please let me think about this, the namespace naming here is shoddy to say the least... GitCommands.Git.Commands
. We can definitely improve here making the API much more pleasant to use.
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 propose the following:
- Remove "Commands" folder and instead create
Commands
type that would contain all implementations for git commands. - Turn
GitCommand
class into a private nested class ofCommands
class. - Remove
GitCommandHelpers
class and move its implementations toCommands
class.
This may sound a little drastic, but I think that would consolidate the API and make it much more cohesive.
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 have implemented all the proposals regarding Commands
type.
4514b06
to
2b6f66e
Compare
40f36f0
to
44cf233
Compare
Fixes #11230 (comment)
Proposed changes
GitCommand
but provide a factory method per git commandGitCommand
via ctorGitTagControllerTest
, needed because the git arguments are validated at onceScreenshots
N/A
Test methodology
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.