-
Notifications
You must be signed in to change notification settings - Fork 392
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
Config properties #1591
Config properties #1591
Conversation
…e provider to the configuration property.
…operty so that it can property override values that initially come from targets or props.
…/Platforms properties in the project
namespace Microsoft.VisualStudio.ProjectSystem.VS.Configuration | ||
{ | ||
[ProjectSystemTrait] | ||
public class ConfigurationConfigurationProjectConfigurationDimensionProviderTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
/// </summary> | ||
/// <param name="args">Information about the configuration dimension value change.</param> | ||
/// <returns>A task for the async operation.</returns> | ||
public override async Task OnDimensionValueChangedAsync(ProjectConfigurationDimensionValueChangedEventArgs args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit convoluted to me - each provider is responsible for writing out the updated values to the MSBuild project file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, that's one of the downsides of adding an extensibility point with full control. To make things even more interesting, add/remove need to be saved at different times than rename due to the order of operations on the CPS side.
/// </summary> | ||
[Export(typeof(IProjectConfigurationDimensionsProvider2))] | ||
[AppliesTo(ProjectCapabilities.ProjectConfigurationsDeclaredDimensions)] | ||
[Order(int.MaxValue -1)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment about the order attribute value in all providers.
await OnConfigurationRenamed(args.Project, args.OldDimensionValue, args.DimensionValue).ConfigureAwait(true); | ||
} | ||
|
||
// await _projectXmlAccessor.ExecuteInWriteLock((msbuildProject) => msbuildProject.Save()).ConfigureAwait(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I was trying to see if I could work around another issue and forgot to delete this.
See #1771 for updated PR post merges. |
This depends on the CPS changes that haven't been checked in yet, https://mseng.visualstudio.com/VSIDEProj/_git/VSIDEProj.CPS/pullrequest/186915?_a=overview
It implements dimension providers for configuration and platform based on 'Configurations' and 'Platforms' project property.
There are still 2 issues I'm investigating in the scenario but those appear to be existing issues as I'm hitting them without any changes in either CPS or the project system.
There will also a follow up PR on the SDK as the defaults will be set there