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

Added GitVersion.Abstractions module #3451

Merged
merged 8 commits into from
Mar 28, 2023

Conversation

arturcic
Copy link
Member

Moving most of the interfaces from Core to Abstractions, made modules depend on Abstractions instead of Core

@arturcic arturcic force-pushed the feature/abstractions branch from 8103077 to d91c3ac Compare March 28, 2023 16:01
@arturcic arturcic force-pushed the feature/abstractions branch from d91c3ac to ea49bfa Compare March 28, 2023 16:04
@arturcic arturcic added this to the 6.x milestone Mar 28, 2023
@arturcic arturcic merged commit 7feae1d into GitTools:main Mar 28, 2023
@mergify
Copy link
Contributor

mergify bot commented Mar 28, 2023

Thank you @arturcic for your contribution!

@arturcic arturcic deleted the feature/abstractions branch March 28, 2023 16:46
@asbjornu
Copy link
Member

Sorry, but I don’t agree with this change. We don’t need to move interfaces out of Core, that makes it not be the core anymore.

@arturcic
Copy link
Member Author

Core will change to Calculation, it will contain only the version calculation logic, eventually

@arturcic
Copy link
Member Author

Sorry, but I don’t agree with this change. We don’t need to move interfaces out of Core, that makes it not be the core anymore.

The Abstractions module has the contracts defined, that means the modules we have (output and agents for now) will depend on the contracts. Now the implementations of those interfaces are defined in the specific module.

@arturcic
Copy link
Member Author

@asbjornu please have a look here #3407 (comment)

@asbjornu
Copy link
Member

Core will change to Calculation, it will contain only the version calculation logic, eventually

Don't you consider version calculation to be "core" to what GitVersion does?

The Abstractions module has the contracts defined, that means the modules we have (output and agents for now) will depend on the contracts. Now the implementations of those interfaces are defined in the specific module.

Sounds like the separation we already had with Core?

@arturcic
Copy link
Member Author

Don't you consider version calculation to be "core" to what GitVersion does?

Yes, and only version calculation should be in core, all the other modules - like output, agents, maybe configuration, should not belong to core in my opinion. The version calculation code should know about the Options and the EffectiveConfiguration as inputs, and those should be provided/computed by the 'external modules', same for the output - GitVersionVariables, that is used by 'external modules'

Basically the Core takes input and gives back the output. It should not be aware how the input/output is prepared/used.

Now if we keep those interfaces and implementations in the Core then it has too many responsibilities. That's why I was saying that Core will eventually become Calculation. That's why I introduced the Abstractions (can be renamed to Contracts).

@arturcic
Copy link
Member Author

I can revert this PR if you want to have a broader discussion on this refactoring.

@asbjornu
Copy link
Member

asbjornu commented Mar 28, 2023

Hm, I think I agree with almost everything you just wrote, so I have to take a more thorough look at this to understand exactly what it is you're writing compared to what this PR is actually doing.

@arturcic
Copy link
Member Author

The PR is moving to that direction, not yet there

@arturcic arturcic restored the feature/abstractions branch March 28, 2023 20:54
@arturcic arturcic removed this from the 6.x milestone Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants