Skip to content

Conversation

mstv
Copy link
Member

@mstv mstv commented Sep 30, 2023

Fixes #11230 (comment)

Proposed changes

  • Do not subclass GitCommand but provide a factory method per git command
  • Provide values of - formerly abstract - properties of GitCommand via ctor
  • Complete the mock up for GitTagControllerTest, needed because the git arguments are validated at once

Screenshots

N/A

Test methodology

  • adapted tests

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.

@mstv mstv self-assigned this Sep 30, 2023
Reset
}

public static partial class GitCommandFactory
Copy link
Member

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.

Copy link
Member Author

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(...)

Copy link
Member

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.

Copy link
Member

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 of Commands class.
  • Remove GitCommandHelpers class and move its implementations to Commands class.

This may sound a little drastic, but I think that would consolidate the API and make it much more cohesive.

Copy link
Member Author

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.

@ghost ghost added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity and removed 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity labels Oct 1, 2023
@mstv mstv added this to the vNext milestone Oct 7, 2023
@mstv mstv force-pushed the feature/simplify_gitcommand branch from 4514b06 to 2b6f66e Compare October 7, 2023 22:03
@mstv mstv force-pushed the feature/simplify_gitcommand branch from 40f36f0 to 44cf233 Compare October 8, 2023 22:22
@mstv mstv merged commit 44cf233 into gitextensions:master Oct 8, 2023
@mstv mstv deleted the feature/simplify_gitcommand branch October 8, 2023 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants