Rename types/variables in the product/test code to reflect package source mapping #4281

kartheekp-ms · 2021-09-27T23:45:08Z

Bug

Fixes: NuGet/Home#11216

Regression? Last working version:

Description

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
This is a follow-up PR to address Rename packageNamespaces to packageSourceMapping, namespace to package and id to pattern #4234 (review) comment. Renamed all the types, variables, and comments to the best of my search capability in the product/test code to reflect package source mapping instead of package namespaces.
Tests
- N/A
Documentation
- N/A

…mapping

src/NuGet.Core/NuGet.Protocol/NuGet.Protocol.csproj

kartheekp-ms · 2021-09-28T00:52:30Z

src/NuGet.Core/NuGet.Configuration/PackageNamespaces/PackageNamespacesProvider.cs

-
-namespace NuGet.Configuration
-{
-    internal class PackageNamespacesProvider


I meant to rename this file but I forgot what user actions in the IDE lead to deleting this file and creating a new one with the name PackageSourceMappingProvider.cs

...Get.Clients/NuGet.PackageManagement.PowerShellCmdlets/Utility/PackageSourceMappingUtility.cs

nkolev92

Seems pretty thorough to me.
Some minor feedback.

nkolev92 · 2021-09-28T00:46:32Z

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingConfiguration.cs

@@ -7,7 +7,7 @@

 namespace NuGet.Configuration
 {
-    public class PackageNamespacesConfiguration
+    public class PackageSourceMappingConfiguration


naming: Should this simply be PackageSourceMapping ?

Configuration might be superfluous?
Hoping @zivkan can provide some feedback as well.

PackageSourceMappingConfiguration is too long, so above suggestion is good for me. If we do above then need to do same for PackageSourceMappingConfigurationUtility and other variables too.

Yes, I like PackageSourceMapping. Pretending that I'm working on something in restore, unrelated to the package source mapping feature, I feel like it should be sufficiently obvious it's a class that holds a mapping between package prefixes are package sources. Therefore, I don't think the name Configuration adds clarification. I also can't think why there'd be any PackageSourceMapping class in any assembly other than NuGet.Configuration, so I think it passes all the naming tests I can think of.

I renamed the type to PackageSourceMapping in the latest commit which already passed all CI checks.

src/NuGet.Core/NuGet.Configuration/Settings/SettingElementType.cs

nkolev92 · 2021-09-28T00:56:18Z

src/NuGet.Core/NuGet.DependencyResolver.Core/Strings.resx

@@ -121,11 +121,11 @@
    <value>The feed '{0}' lists package '{1}' but multiple attempts to download the nupkg have failed. The feed is either invalid or required packages were removed while the current operation was in progress. Verify the package exists on the feed and try again.</value>
  </data>
  <data name="Log_MatchingSourceFoundForPackage" xml:space="preserve">
-    <value>Package namespace matches found for package ID '{0}' are: '{1}'.</value>
+    <value>Package source mapping pattern matches found for package ID '{0}' are: '{1}'.</value>


You might've already done it, but I'd rather provide the feedback than risk us missing something.

Consider search for the word namespace in all resx files in our repo.

My guess is that we shouldn't have any hits.

I did search for namespace in all the files in the repo (that includes resx files) and could not find a keyword namespaces in the context of source mapping feature.

Awesome! Thanks for being so thorough! Very important with a PR like this.

nkolev92 · 2021-09-28T00:56:51Z

src/NuGet.Core/NuGet.DependencyResolver.Core/Strings.resx

    <comment>{0} - Package id {1} - list of sources</comment>
  </data>
  <data name="Log_NoMatchingSourceFoundForPackage" xml:space="preserve">
-    <value>Package namespace match not found for package ID '{0}'.</value>
+    <value>Package source mapping pattern match not found for package ID '{0}'.</value>


Is pattern still necessary here?

removed the pattern from the log message

nkolev92 · 2021-09-28T00:57:49Z

src/NuGet.Core/NuGet.PackageManagement/Strings.resx

-  <data name="PackageNamespaceNoMatchFound" xml:space="preserve">
-    <value>Package namespace match not found for package ID '{0}'</value>
+  <data name="PackageSourceMappingPatternNoMatchFound" xml:space="preserve">
+    <value>Package source mapping pattern match not found for package ID '{0}'</value>


If you agree with my feedback on removing pattern above, it'd apply here as well.

removed pattern from the log message.

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreRequest.cs

erdembayar · 2021-09-28T01:02:33Z

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMappingConfiguration.cs


-            foreach (PackageNamespacesSourceItem packageSourceNamespaceItem in packageNamespacesProvider.GetPackageSourceNamespaces())
+            foreach (PackageSourceMappingSourceItem packageSourceNamespaceItem in packageSourceMappingProvider.GetPackageSourceMappingItems())


Maybe GetPackageSourceMappingPatterns could be better description of what it does?

PackageSourceMappingProvider is an internal type. I think GetPackageSourceMappingItems is more apt because it returns a list of type PackageSourceMappingSourceItem. Please let me know if you disagree.

erdembayar · 2021-09-28T01:08:44Z

src/NuGet.Core/NuGet.Configuration/Settings/Items/PackageSourceMappingSourceItem.cs

        /// </summary>
-        public IList<NamespaceItem> Namespaces { get; }
+        public IList<PackagePatternItem> Patterns { get; }


Suggested change

public IList<PackagePatternItem> Patterns { get; }

public IList<PackagePatternItem> SourcePatterns { get; }

Given that the type name PackageSourceMappingSourceItem has Source word twice, I think just Patterns seems okay to me. Earlier it was Namespaces and I just replaced it with Patterns because the XML element <packageSource> contains a list of patterns. Please let me know your feedback. cc @nkolev92

I like Patterns.
If we were going to make it verbose, I'd argue PackagePatterns is the right name over SourcePatterns.

I like Patterns too. If I use PackagePatterns the term Package may be repeated twice once in the type name and in the property name.

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetRestoreCommandTest.cs

test/NuGet.Core.Tests/NuGet.Configuration.Test/PackageSourceMappingConfigurationTests.cs

...uGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/DependencyGraphSpecRequestProvider.cs

nkolev92

1 important comment and a bunch of little ones.

src/NuGet.Clients/NuGet.CommandLine/Commands/InstallCommand.cs

nkolev92 · 2021-09-28T17:01:06Z

src/NuGet.Clients/NuGet.CommandLine/Commands/RestoreCommand.cs

@@ -382,9 +382,9 @@ private async Task<IReadOnlyList<RestoreSummary>> PerformNuGetV2RestoreAsync(Pac
                cacheContext.NoCache = NoCache;
                cacheContext.DirectDownload = DirectDownload;

-                PackageNamespacesConfiguration packageNamespacesConfiguration = PackageNamespacesConfiguration.GetPackageNamespacesConfiguration(Settings);
+                PackageSourceMapping packageSourceMappingConfiguration = PackageSourceMapping.GetPackageSourceMappingConfiguration(Settings);


nit: var name. Not critical.

This applies in many spots. You can consider changing these.

fixed it in all the places using find and replace.

Seems like there's still a bunch of these instances. Can you please double check that you committed those changes?

Bumping this.

I think I got it right this time and fixed in other places too.

nkolev92 · 2021-09-28T17:02:49Z

src/NuGet.Core/NuGet.Configuration/PackageSourceMapping/PackageSourceMapping.cs

        /// <exception cref="ArgumentNullException"> if <paramref name="settings"/> is null.</exception>
-        public static PackageNamespacesConfiguration GetPackageNamespacesConfiguration(ISettings settings)
+        public static PackageSourceMapping GetPackageSourceMappingConfiguration(ISettings settings)


This should be GetPackageSourceMapping.

I thought about it when I renamed PackageSourceMappingConfiguration type to PackageSourceMapping but forgot to update the method name. thanks, Nikolche. Fixed in the next commit.

nkolev92 · 2021-09-28T17:04:36Z

src/NuGet.Core/NuGet.Configuration/Settings/Items/PackageSourceMappingSourceItem.cs

        /// </summary>
-        public IList<NamespaceItem> Namespaces { get; }
+        public IList<PackagePatternItem> Patterns { get; }


I like Patterns.
If we were going to make it verbose, I'd argue PackagePatterns is the right name over SourcePatterns.

zivkan

I looked only at the PublicAPI txt files, as they're more or less the only breaking changes. Everything else we can refactor in the future without API changes.

zivkan · 2021-09-28T17:21:06Z

src/NuGet.Core/NuGet.Configuration/PublicAPI.Unshipped.txt

+NuGet.Configuration.PackagePatternItem.PackagePatternItem(string pattern) -> void
+NuGet.Configuration.PackagePatternItem.Pattern.get -> string
+NuGet.Configuration.PackageSourceMapping
+NuGet.Configuration.PackageSourceMapping.GetConfiguredPackageSources(string term) -> System.Collections.Generic.IReadOnlyList<string>


I think packageId makes more sense than term.

Fixed in the latest commit.

Actually mostly packagePrefix than packageId, but I'm ok with packageId.

Isn't that API the package id exactly?
It's getting the sources for a specific package.

Yes Nikolche that is correct, exact packageId is passed as parameter to this method to find out package sources associated with the longest matching prefix.

zivkan · 2021-09-28T17:24:07Z

src/NuGet.Core/NuGet.Configuration/PublicAPI.Unshipped.txt

+NuGet.Configuration.PackageSourceMapping.IsEnabled.get -> bool
+NuGet.Configuration.PackageSourceMappingSourceItem
+NuGet.Configuration.PackageSourceMappingSourceItem.PackageSourceMappingSourceItem(string name, System.Collections.Generic.IEnumerable<NuGet.Configuration.PackagePatternItem> packagePatternItems) -> void
+NuGet.Configuration.PackageSourceMappingSourceItem.Patterns.get -> System.Collections.Generic.IList<NuGet.Configuration.PackagePatternItem>


Does the order of patterns matter? If not, then ICollection<T> might be a more minimalist interface for how the result is used.

Fixed in the latest commit.

I disagree with this.

Using too generic interfaces like that can lead to lots of accidental and unnecessary enumerations.
We should stick with IList imo.

reverted the last commit where I changed the Patterns type to ICollection

…eSourceMappingUtility

nkolev92 · 2021-09-28T19:39:30Z

src/NuGet.Clients/NuGet.CommandLine/Commands/InstallCommand.cs

@@ -371,9 +371,9 @@ private CommandLineSourceRepositoryProvider GetSourceRepositoryProvider()
                    resolutionContext.SourceCacheContext.NoCache = NoCache;
                    resolutionContext.SourceCacheContext.DirectDownload = DirectDownload;

-                    PackageNamespacesConfiguration packageNamespacesConfiguration = PackageNamespacesConfiguration.GetPackageNamespacesConfiguration(Settings);
+                    var packageSourceMappingConfiguration = PackageSourceMapping.GetPackageSourceMapping(Settings);


nit: var naming.

Note that searching for packageSourceMappingConfiguration reveals 27 hits just on GitHub.

I didn't change the variable names but focused on using var keyword to avoid breaking references to it. I will create a follow-up PR and fix the variable names, if needed.

My comment is not about the var keyword, it's about the naming.

It's var packageSourceMappingConfiguration, but should be var packageSourceMapping

sure. I will fix the variable naming in the follow-up issue. https://github.com/NuGet/Client.Engineering/issues/1219

Kartheek Penagamuri added 5 commits September 27, 2021 14:26

rename types and variables from package namespaces to package source …

ab1c4ab

…mapping

fix build failures

41619d3

rename few more test names

3be22e4

replace namespace with source mapping

5c30791

fix public api

0aea1d1

kartheekp-ms requested a review from a team as a code owner September 27, 2021 23:45

Kartheek Penagamuri added 2 commits September 27, 2021 17:41

fix and rename dotnet integration test

8aa7d9e

remove unnecessary file

bb41caf

zivkan reviewed Sep 28, 2021

View reviewed changes

src/NuGet.Core/NuGet.Protocol/NuGet.Protocol.csproj Outdated Show resolved Hide resolved

kartheekp-ms commented Sep 28, 2021

View reviewed changes

erdembayar reviewed Sep 28, 2021

View reviewed changes

...Get.Clients/NuGet.PackageManagement.PowerShellCmdlets/Utility/PackageSourceMappingUtility.cs Show resolved Hide resolved

nkolev92 reviewed Sep 28, 2021

View reviewed changes

erdembayar reviewed Sep 28, 2021

View reviewed changes

src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreRequest.cs Outdated Show resolved Hide resolved

erdembayar reviewed Sep 28, 2021

View reviewed changes

Kartheek Penagamuri added 5 commits September 27, 2021 18:25

feedback

ff77a72

rename PackageSourceMappingConfiguration to PackageSourceMapping

cd79b8e

rename PackageNamespacesConfiguration to PackageNamespaces

fdcf448

fix source mapping typo

e12cf73

fix log messages

5ed4a82

martinrrm approved these changes Sep 28, 2021

View reviewed changes

fix typos and variable names

1f30df5

kartheekp-ms requested review from nkolev92, zivkan and erdembayar September 28, 2021 16:50

nkolev92 reviewed Sep 28, 2021

View reviewed changes

PR feedback

8f278f3

kartheekp-ms requested a review from nkolev92 September 28, 2021 17:43

zivkan approved these changes Sep 28, 2021

View reviewed changes

change parameter name from term to packageId

779e31f

kartheekp-ms force-pushed the dev-kartheekp-ms-renametypes-packagenamespaces branch from cc2cf41 to 779e31f Compare September 28, 2021 18:50

use var and rename PackageSourceMappingConfigurationUtility to Packag…

bb122bb

…eSourceMappingUtility

nkolev92 approved these changes Sep 28, 2021

View reviewed changes

kartheekp-ms requested a review from aortiz-msft September 28, 2021 20:16

aortiz-msft approved these changes Sep 28, 2021

View reviewed changes

erdembayar approved these changes Sep 28, 2021

View reviewed changes

aortiz-msft merged commit e301669 into dev Sep 28, 2021

aortiz-msft deleted the dev-kartheekp-ms-renametypes-packagenamespaces branch September 28, 2021 21:46

kartheekp-ms mentioned this pull request Sep 28, 2021

rename packageSourceMappingConfiguration variable to packageSourceMapping #4285

Merged

5 tasks


		foreach (PackageNamespacesSourceItem packageSourceNamespaceItem in packageNamespacesProvider.GetPackageSourceNamespaces())
		foreach (PackageSourceMappingSourceItem packageSourceNamespaceItem in packageSourceMappingProvider.GetPackageSourceMappingItems())

	public IList<PackagePatternItem> Patterns { get; }
	public IList<PackagePatternItem> SourcePatterns { get; }

Rename types/variables in the product/test code to reflect package source mapping #4281

Rename types/variables in the product/test code to reflect package source mapping #4281

Conversation

kartheekp-ms commented Sep 27, 2021 • edited Loading

Bug

Description

PR Checklist

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms commented Sep 27, 2021 •

edited

Loading

kartheekp-ms Sep 28, 2021 •

edited

Loading

nkolev92 left a comment •

edited

Loading

kartheekp-ms Sep 28, 2021 •

edited

Loading