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 global properties to SdkResolverContext #3617

Closed
wants to merge 1 commit into from

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Aug 16, 2018

This will allow the NuGet SDK resolver to read the global property NuGetInteractive so it can prompt the user.

This will allow the NuGet SDK resolver to read the global property `NuGetInteractive` so it can prompt the user.
@jeffkl jeffkl changed the base branch from master to vs15.9 August 16, 2018 18:20
@jeffkl
Copy link
Contributor Author

jeffkl commented Aug 16, 2018

Please review ASAP to get this in so I can update NuGet's SDK resolver.

@AndyGerlicher
Copy link
Contributor

@nguerrera any thoughts or concerns with this?

@rainersigwald
Copy link
Member

I'm not a fan of this approach, as I mentioned in #2095 (comment).

@nguerrera
Copy link
Contributor

I've asked for it a few times, See #2095.

@rainersigwald
Copy link
Member

Though restricting to global properties helps, I'm not sure it's enough since you could have global properties set in a p2p reference.

@nguerrera
Copy link
Contributor

At the end of #2095, I asked if global properties alone would be OK. Seems we have varying answers still. :)

@nguerrera
Copy link
Contributor

I have no objections and you can close #2095 if you decide to take it. I don't have a pressing need for this either.

@@ -2132,7 +2132,7 @@ private List<ProjectRootElement> ExpandAndLoadImports(string directoryOfImportin
var projectPath = _data.GetProperty(ReservedPropertyNames.projectFullPath)?.EvaluatedValue;

// Combine SDK path with the "project" relative path
sdkResult = _sdkResolverService.ResolveSdk(_submissionId, importElement.ParsedSdkReference, _evaluationLoggingContext, importElement.Location, solutionPath, projectPath);
sdkResult = _sdkResolverService.ResolveSdk(_submissionId, importElement.ParsedSdkReference, _evaluationLoggingContext, importElement.Location, solutionPath, projectPath, _data.GlobalPropertiesDictionary.ToDictionary());
Copy link
Contributor

Choose a reason for hiding this comment

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

This will allocate, and if there's many global properties it will allocate a lot. But I don't see another way around, save for moving to immutable collections

@@ -81,14 +81,15 @@ public override void PacketReceived(int node, INodePacket packet)
}

/// <inheritdoc cref="ISdkResolverService.ResolveSdk"/>
public override SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath)
public override SdkResult ResolveSdk(int submissionId, SdkReference sdk, LoggingContext loggingContext, ElementLocation sdkReferenceLocation, string solutionPath, string projectPath, IDictionary<string, string> globalProperties)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would IReadOnlyDictionary make better sense?

@jeffkl
Copy link
Contributor Author

jeffkl commented Aug 16, 2018

Nevermind...

@jeffkl jeffkl closed this Aug 16, 2018
@jeffkl jeffkl deleted the globalpropssdkresolvercontext branch October 9, 2019 21:25
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

5 participants