Skip to content

Configuration Updates #341

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

Conversation

JakeGinnivan
Copy link
Contributor

@yannisgu this branch is probably useful to you when I finish it. I think without the core configuration changes contained in this PR it will make updating the branch finders much harder (I think that is also why most of your tests are failing).

Thoughts on the direction in this PR?

@@ -98,36 +90,16 @@ public override bool Execute()
{
try
{
CachedVersion versionAndBranch;
Tuple<CachedVersion, GitVersionContext> versionAndBranch;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a massive fan of this, but it is a temporary hack to allow me to keep the MSBuild tasks in a working and compatible state.

With the Context being responsible for calculating effective configuration it needs to be exposed with the version for use in the variable provider.

As we get further down this refactoring hopefully more of this can all move into Core

@yannisgu
Copy link
Contributor

yannisgu commented Jan 9, 2015

I like the idea of having a merged EffectiveConfiguration object.

However I would like to have a more dynamic solution here with reflection. Instead of calculating every property one by one in CalculateEffectiveConfiguration, I would add a constructor to EffectiveConfiguration which takes a EffectiveConfiguration and a BranchConfiguration. Then both objects gets merged by reflection (iterating over properties of EffectiveConfiguration, if it exists on BranchConfiguration and is not null, get from it).

@JakeGinnivan
Copy link
Contributor Author

Could do. It would take 3 things, Config (which is the top level config), BranchConfig and EffectiveConfiguration (which is the defaults/fallback for everything).

Will give that a go

@JakeGinnivan JakeGinnivan force-pushed the VariableProviderModes branch from 007e466 to 952659f Compare January 9, 2015 11:14
@JakeGinnivan
Copy link
Contributor Author

@yannisgu I had a crack at the more dynamic effective configuration, at the moment I think I am happy to hard code it. At least until we get this closer to the end state

@JakeGinnivan JakeGinnivan changed the title [WIP] Configuration Updates Configuration Updates Jan 9, 2015
@JakeGinnivan
Copy link
Contributor Author

@yannisgu this and #342 are ready to go I think.

#342 needs to be merged first. Once we merge them I think we rebase your branch on top and it should help you get tests passing

@yannisgu
Copy link
Contributor

yannisgu commented Jan 9, 2015

Yeah looks good 👍

:shipit:

@JakeGinnivan JakeGinnivan force-pushed the VariableProviderModes branch from 8101e16 to b9b2a2b Compare January 9, 2015 19:02
JakeGinnivan added a commit that referenced this pull request Jan 9, 2015
@JakeGinnivan JakeGinnivan merged commit 5d86d09 into GitTools:BranchSpecificConfiguration Jan 9, 2015
@JakeGinnivan JakeGinnivan deleted the VariableProviderModes branch January 9, 2015 19:12
@JakeGinnivan JakeGinnivan restored the VariableProviderModes branch January 10, 2015 16:37
@JakeGinnivan JakeGinnivan deleted the VariableProviderModes branch January 18, 2015 11:10
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.

2 participants