-
Notifications
You must be signed in to change notification settings - Fork 162
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
Move NuGet API usage to its own extension #824
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,10 @@ | |
"Extensions": { | ||
"Source": "https://upgradeassistant.blob.core.windows.net/extensions/index.json", | ||
|
||
"required": [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this naming feels a little confusing. almost feels like something that is default is required? or maybe we move the default extension under required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is "required" because everything will need it. Default extensions are used to augment certain services but not needed everywhere. Long term, it may be good to define extensions on a "per command" usage; ie, identify commands that would only be used in analyze/upgrade/etc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cool! |
||
"nuget" | ||
], | ||
|
||
"Default": [ | ||
"default", | ||
"vb", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Collections.Generic; | ||
|
||
namespace Microsoft.DotNet.UpgradeAssistant | ||
{ | ||
public interface ITargetFrameworkCollection : IReadOnlyCollection<TargetFrameworkMoniker> | ||
{ | ||
void SetTargetFramework(TargetFrameworkMoniker tfm); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,14 +46,21 @@ public ExtensionProvider( | |
|
||
_extensions = new Lazy<IEnumerable<ExtensionInstance>>(() => | ||
{ | ||
var list = new List<ExtensionInstance>(); | ||
|
||
var opts = options.Value; | ||
|
||
if (!opts.LoadExtensions) | ||
// Required extensions are required for all commands UA may handle, as opposed to other extensions that augment certain commands | ||
foreach (var path in opts.RequiredExtensions) | ||
{ | ||
return Enumerable.Empty<ExtensionInstance>(); | ||
LoadPath(path, isDefault: true); | ||
} | ||
|
||
var list = new List<ExtensionInstance>(); | ||
// Required extensions must load, otherwise they may be turned off | ||
if (!opts.LoadExtensions) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need a LoadExtensions option? I'm not sure it's necessary and if we removed it, this code would simplify a bit as default and required extensions wouldn't have to be distinguished anymore. The only downside I can think of to removing it is that users won't be able to disable any default extensions, but I haven't seen a use case where that would be necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When managing extensions, it adds extra time that this prevents. The extension management doesn't need all the random extensions people may be adding, but it will need the NuGet one |
||
{ | ||
return list; | ||
} | ||
|
||
foreach (var path in opts.ExtensionPaths) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System; | ||
|
||
namespace Microsoft.DotNet.UpgradeAssistant.MSBuild | ||
{ | ||
internal class Factories | ||
{ | ||
private readonly Func<IUpgradeContext, IProject, INuGetReferences> _nugetReferenceFactory; | ||
private readonly Func<IProjectFile, ITargetFrameworkCollection> _tfmCollectionFactory; | ||
private readonly Func<string, ISolutionInfo> _infoGenerator; | ||
|
||
public Factories( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm overlooking something here. I'm trying to figure out how this type is instantiated. I see that it's registered as a transient DI service, but I don't see where the Funcs it needs as arguments are registered. Can you help me figure out where the nugetReferenceFactory and tfmCollectionFactory implementations come from? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the beautiful features of Autofac: https://autofac.readthedocs.io/en/develop/resolve/relationships.html |
||
Func<IUpgradeContext, IProject, INuGetReferences> nugetReferenceFactory, | ||
Func<IProjectFile, ITargetFrameworkCollection> tfmCollectionFactory, | ||
Func<string, ISolutionInfo> infoGenerator) | ||
{ | ||
_nugetReferenceFactory = nugetReferenceFactory; | ||
_tfmCollectionFactory = tfmCollectionFactory; | ||
_infoGenerator = infoGenerator; | ||
} | ||
|
||
public INuGetReferences CreateNuGetReferences(IUpgradeContext context, IProject project) => _nugetReferenceFactory(context, project); | ||
|
||
public ITargetFrameworkCollection CreateTfmCollection(IProjectFile project) => _tfmCollectionFactory(project); | ||
|
||
public ISolutionInfo CreateSolutionInfo(string name) => _infoGenerator(name); | ||
} | ||
} |
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.
I believe the setting of
PackageSourcePath
got lost in this PR. It would explain why changing package sources innuget.config
doesn't work anymore.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.
I got the same issue, 0.4.421302 will not follow my nuget.config but 0.2.241603 (a version that is before the above PR) will follow my nuget.config.