Move restore property and item constants to NuGet.Commands #6392
Conversation
| @@ -632,9 +632,6 @@ NuGet.ProjectManagement.Preprocessor | |||
| NuGet.ProjectManagement.Preprocessor.Preprocessor() -> void | |||
| ~NuGet.ProjectManagement.Preprocessor.RevertFileAsync(System.Func<System.Threading.Tasks.Task<System.IO.Stream>> streamTaskFactory, string targetPath, System.Collections.Generic.IEnumerable<NuGet.ProjectManagement.InternalZipFileInfo> matchingFiles, NuGet.ProjectManagement.IMSBuildProjectSystem projectSystem, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.Task | |||
| ~NuGet.ProjectManagement.Preprocessor.TransformFileAsync(System.Func<System.Threading.Tasks.Task<System.IO.Stream>> streamTaskFactory, string targetPath, NuGet.ProjectManagement.IMSBuildProjectSystem projectSystem, System.Threading.CancellationToken cancellationToken) -> System.Threading.Tasks.Task | |||
| NuGet.ProjectManagement.ProjectBuildProperties | |||
| NuGet.ProjectManagement.ProjectItemProperties | |||
| NuGet.ProjectManagement.ProjectItems | |||
There was a problem hiding this comment.
@nkolev92 (or indeed anyone else), do you think we should add type forwarding for these classes?
NuGet.PackageManagement is just such an unlikely package for anyone interacting with NuGet to use, so it seems incredibly unlikely that anyone actually depends on these APIs. Plus, since they're const string, I'm not even use if there's a runtime dependency on the type, or if the compiler copies the const string into the assembly being compiled.
There was a problem hiding this comment.
I'd say tooling disclaimer applies, mark the issue, make sure it's in the release notes and it's all good.
| NuGet.Commands.Restore.Utility.PackageSpecFactory | ||
| NuGet.ProjectManagement.ProjectItemProperties |
There was a problem hiding this comment.
As long as the two VS project system implementations use these types, they still need to be public APIs.
If we're willing to break public APIs, then in the future when we delete the old PackageSpec building methods and only have PackageSpecFactory left, then we could consider making these internal. But at the same time, the tech debt of public const strings that can't be changed for breaking changes reasons is very low.
There was a problem hiding this comment.
We've always had trouble because these types were not in the correct assembly in the first place, so I'd say this is a great long term thing.
dotnet-policy-service
bot
added
the
Status:No recent activity
zivkan
force-pushed
the
dev-zivkan-move-item-property-constants
branch
from
929a656 to
afffd03
Compare
dotnet-policy-service
bot
removed
the
Status:No recent activity
dotnet-policy-service
bot
added
the
Status:No recent activity
|
This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch. |
Bug
Towards: NuGet/Home#14160
Description
NuGet has string constants for all (or at least the vast majority) of property and items that restore uses to build a PackageSpec. However, those constants were in NuGet.PackageManagement, which is not referenced by msbuild restore, and therefore neither msbuild restore not static graph restore could actually use those constants. Same applies for the newly created PackageSpecFactory added in my previous pull request.
So, this PR moves the classes with the constants to NuGet.Commands, where PackageSpecFactory is, and updates all the string literals to the constants.
This makes the constants available for msbuild and static graph restore as well, but the point of PackageSpecFactory is to be able to delete those implementations, so I didn't update them, to reduce risk of regression and to avoid spending effort code that will be deleted in the future.
Since there's a separate class for item metadata names, I deleted some strings from the project properties class, and had to update references to point to the correct class name.
PR Checklist
Added testsno behaviour changes, existing tests should be sufficient.Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.n/a