Skip to content

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?

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