Skip to content

Add global properties to SdkResolverContext#3617

Closed
jeffkl wants to merge 1 commit intodotnet:vs15.9from
jeffkl:globalpropssdkresolvercontext
Closed

Add global properties to SdkResolverContext#3617
jeffkl wants to merge 1 commit intodotnet:vs15.9from
jeffkl:globalpropssdkresolvercontext

Conversation

@jeffkl
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@nguerrera any thoughts or concerns with this?

@rainersigwald
Copy link
Copy Markdown
Member

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

@nguerrera
Copy link
Copy Markdown
Contributor

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

@rainersigwald
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@nguerrera
Copy link
Copy Markdown
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.


// 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
Copy Markdown
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


/// <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
Copy Markdown
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
Copy Markdown
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.

5 participants