Skip to content
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

Add GitExtensions.Extensibility #11431

Merged
merged 9 commits into from
May 14, 2024
Merged

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Dec 7, 2023

Proposed changes

This change allows for a separation of concerns - shared contracts that affect the main app as well as Git Extensions plugins will now reside in Gitextensions.Extensibility project, and concrete implementations of the contracts reside in the app.
This is a stepping stone in a direction of complete separation of the contracts and future move of those to https://github.com/gitextensions/gitextensions.extensibility project.

I expect further work will be required to clean the contract declarations (e.g., removal of default args as those play tricks on developers) as well as a holistic reassessment of the API surface.

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.

@RussKie RussKie requested a review from a team December 7, 2023 08:00
@ghost ghost assigned RussKie Dec 7, 2023
@mstv
Copy link
Member

mstv commented Dec 7, 2023

There must be a very huge benefit of moving all these interfaces and utilities to a submodule / NuGet package.
Could you elaborate the benefits, please? Where do you draw the line between things exposed to plugins and the internal, more changeable implementation.
They need to compensate for the friction added by switching to/from submodules and by split PRs.

@gerhardol
Copy link
Member

This will make development of the core app annoying, will slow down the app for the benefit of a few plugins. Most moved functionality does not make sense in a module, for instance GitModule have mostly app internal functions.
This will lead to duplicated code.

@mstv
Copy link
Member

mstv commented Dec 8, 2023

It should not be too difficult to directly publish the GE Extensibility NuGet from the GE main repo, shouldn't it?

@gerhardol
Copy link
Member

With this, you know if the plugins require changes after a GE update. On the other hand, there are so much core functionality in Extensibility that the plugins will always have to be updated.
There are easier ways to achieve the strict compatibility without limiting the core app development.
Of course, the plugin functionality could be limited (more like other interfaces?), but it would not be better for plugins.

Adding plugins to an existing app without a designed interface is not smooth.

@gerhardol gerhardol mentioned this pull request Dec 17, 2023
@mstv
Copy link
Member

mstv commented Jan 8, 2024

this is precisely why I want the complete segregation of the interfaces and implementations so that we have design discussions and come up with API that both age gracefully with time, provide a solid foundation for extensibility and provide coherent and meaningful contract.

I fully support this! But I have concerns / objections to move them out of the repo. I still think my above comments are clear.

@RussKie
Copy link
Member Author

RussKie commented Jan 8, 2024 via email

@RussKie RussKie force-pushed the ge.extensibility branch 6 times, most recently from 4b0708b to 5f40fa4 Compare May 12, 2024 14:38
@RussKie RussKie marked this pull request as ready for review May 12, 2024 14:42
@RussKie
Copy link
Member Author

RussKie commented May 12, 2024

I rebased the changeset and split it in a way that should make the review easy. You can safely skip over 598072e, c4c65da and f9c9244 as those were mechanical auto-fixer changes.

Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
I have fixed the first three comments myself.

Move API contracts and definitions to GitExtensions.Extensibility project.
@RussKie RussKie merged commit 778a4c9 into gitextensions:master May 14, 2024
4 checks passed
@RussKie RussKie deleted the ge.extensibility branch May 14, 2024 21:06
@RussKie
Copy link
Member Author

RussKie commented May 14, 2024

Thank you @mstv

@RussKie RussKie added this to the vNext milestone May 21, 2024
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.

None yet

3 participants