Add package namespace configuration reading and processing logic #4009

nkolev92 · 2021-04-20T17:40:54Z

Bug

Fixes: NuGet/Home#10725

Regression? Last working version:

Description

Design at https://github.com/NuGet/Home/blob/package-namespaces/proposed/2021/PackageNamespaces.md.

Add the required models to read the current iteration of the configuration for namespaces.

This adds a few things:

Items for namespace and packageSource under packageNamespaces.
Reading and updating provider for namespaces.
A configuration model that could be used during restore time.

Note that these APIs are not final, especially the PackageNamespaceConfiguration one. I just don't want to overdo it before we under the needs of the APIs better.

Wait a feature branch?

Yes - https://github.com/NuGet/Client.Engineering/pull/910

PR Checklist

dominoFire

Looks good. Some comments on constructor.

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

test/NuGet.Core.Tests/NuGet.Configuration.Test/SettingsFileParsingTests/NamespaceItemTests.cs

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

dominoFire · 2021-04-20T18:07:34Z

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

+            return newItem;
+        }
+
+        public override bool Equals(object other)


Do you need to implement IEquatable<> ?

Technically, there is no need for it.
In practice, it'd only save in boxing. Not sure it's be a huge gain in the context of configurations.

At least for me, it helps me in understanding code.

I see that PackageSource element implements IEquatable interface

NuGet.Client/src/NuGet.Core/NuGet.Configuration/PackageSource/PackageSource.cs

Line 13 in f298d71

public class PackageSource : IEquatable<PackageSource>

Not sure how implementing IEquatable improves code understanding here?

I see that PackageSource element implements IEquatable interface

I get that. If we look at many other items like:

https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Configuration/Settings/Items/AddItem.cs
https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Configuration/Settings/Items/AuthorItem.cs
https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Configuration/Settings/Items/CertificateItem.cs
https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Core/NuGet.Configuration/Settings/Items/ClearItem.cs

They don't implement it. So precedent is spotty.

IEquatable was primarily added for structs, where Equals compares the ref instead.

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

nkolev92 · 2021-04-20T21:10:18Z

Thanks @dominoFire, addressed the feedback.

kartheekp-ms

Great start Nikolche. Few comments to start with.

kartheekp-ms · 2021-04-20T21:03:35Z

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

+                    _settings.Remove(ConfigurationConstants.PackageNamespaces, packageSourceNamespace);
+                }
+                // An error means the item doesn't exist or is in a machine wide config, therefore just ignore it
+                catch { }


Suggested change

catch { }

catch (InvalidOperationException) { }

Can we catch InvalidOperationException instead of a generic exception because _settings.Remove throws only InvalidOperationException exception in few cases?

It could throw some IO exceptions if the config is not editable.
We wouldn't do anything with the exception anyways.

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

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

kartheekp-ms · 2021-04-20T22:49:21Z

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

+    {
+        public override string ElementName => ConfigurationConstants.Namespace;
+
+        public string Id


I think we should add additional validation to Id attribute because it is a prefix of Package ID which has few constraints.

NuGet.Client/src/NuGet.Core/NuGet.Packaging/PackageCreation/Utility/PackageIdValidator.cs

Lines 13 to 16 in f298d71

public const int MaxPackageIdLength = 100;

private static readonly Regex IdRegex = new Regex(pattern: @"^\w+([.-]\w+)*$",

options: RegexOptions.IgnoreCase | RegexOptions.ExplicitCapture | RegexOptions.CultureInvariant,

matchTimeout: TimeSpan.FromSeconds(10));

I meant to create a follow up issue for this.

Created an issue: NuGet/Home#10783

A couple of reasons:

I want to unblock the next set of work.

We haven't explicitly designed for the invalid namespace experience. Do we fail early? Warn and continue? Ignore? We can cover this Thursday.

Given that this is a configuration only change, it wasn't super clear where the most convenient location to validate this is.

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

kartheekp-ms · 2021-04-20T23:04:17Z

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

+            return newItem;
+        }
+
+        public override bool Equals(object other)


I see that PackageSource element implements IEquatable interface

NuGet.Client/src/NuGet.Core/NuGet.Configuration/PackageSource/PackageSource.cs

Line 13 in f298d71

public class PackageSource : IEquatable<PackageSource>

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

nkolev92 · 2021-04-21T01:54:18Z

Ready for another review; @kartheekp-ms @dominoFire

erdembayar

Glad to see this one is going forward.
To start I have few comments.

erdembayar · 2021-04-21T00:04:28Z

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

+            return packageNamespacesSection.Items.OfType<PackageSourceNamespacesItem>().ToList();
+        }
+
+        public void Remove(IReadOnlyList<PackageSourceNamespacesItem> packageSourceNamespaces)


Are remove/update scenarios are covered in spec?
In what scenarios is it going to happen?

There will be a command soon. Tracked in NuGet/Home#10735.

Implementing the Get/Add/Update/Remove in one pass seemed like a natural extension.
Keep in mind that this API is currently internal only, so it carries no risk as of now.

We will need it by the time we ship the feature.

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

dominoFire

Signing off. Please address comment on string resource. Thanks!

src/NuGet.Core/NuGet.Configuration/Resources.resx

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

test/NuGet.Core.Tests/NuGet.Configuration.Test/SettingsFileParsingTests/NamespaceItemTests.cs

test/NuGet.Core.Tests/NuGet.Configuration.Test/SettingsFileParsingTests/OwnersItemTests.cs

… clean-up

erdembayar

Good job.
One tiny comment.

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

nkolev92 requested a review from a team as a code owner April 20, 2021 17:40

dominoFire reviewed Apr 20, 2021

View reviewed changes

nkolev92 requested a review from dominoFire April 20, 2021 21:10

kartheekp-ms reviewed Apr 20, 2021

View reviewed changes

nkolev92 requested a review from kartheekp-ms April 21, 2021 01:54

erdembayar reviewed Apr 21, 2021

View reviewed changes

nkolev92 requested a review from erdembayar April 21, 2021 05:45

dominoFire approved these changes Apr 21, 2021

View reviewed changes

nkolev92 added 9 commits April 21, 2021 11:51

Add package namespace configuration reading and processing logic

718fabf

Address formatting feedback, add more testcases

5d8c860

Fix api declaration

badedac

Fix resources, merge a few strings

9cecb64

Rename PackageSourceNamespaceItem to PackageNamespaceSourceItem, more…

a5df917

… clean-up

Commit forgotten files

bec5686

Fix the comparer

045520e

Use explicit types instead of var. Small test refacotring

3e72266

Cleanup

06362a5

nkolev92 force-pushed the dev-nkolev92-namespacesConfiguration branch from cbce607 to 06362a5 Compare April 21, 2021 19:00

erdembayar approved these changes Apr 22, 2021

View reviewed changes

src/NuGet.Core/NuGet.Configuration/Settings/Items/NamespaceItem.cs Outdated Show resolved Hide resolved

Remove unused import"

88366d0

nkolev92 merged commit ea2814d into dev-feature-packageNamespaces Apr 22, 2021

nkolev92 deleted the dev-nkolev92-namespacesConfiguration branch April 22, 2021 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add package namespace configuration reading and processing logic #4009

Add package namespace configuration reading and processing logic #4009

nkolev92 commented Apr 20, 2021 •

edited

Loading

dominoFire left a comment

dominoFire Apr 20, 2021

nkolev92 Apr 20, 2021

dominoFire Apr 20, 2021

kartheekp-ms Apr 20, 2021

nkolev92 Apr 21, 2021 •

edited

Loading

nkolev92 commented Apr 20, 2021

kartheekp-ms left a comment

kartheekp-ms Apr 20, 2021

nkolev92 Apr 21, 2021

kartheekp-ms Apr 20, 2021

nkolev92 Apr 21, 2021

kartheekp-ms Apr 20, 2021

nkolev92 commented Apr 21, 2021

erdembayar left a comment

erdembayar Apr 21, 2021

nkolev92 Apr 21, 2021

dominoFire left a comment

erdembayar left a comment

	public const int MaxPackageIdLength = 100;
	private static readonly Regex IdRegex = new Regex(pattern: @"^\w+([.-]\w+)*$",
	options: RegexOptions.IgnoreCase \| RegexOptions.ExplicitCapture \| RegexOptions.CultureInvariant,
	matchTimeout: TimeSpan.FromSeconds(10));

Add package namespace configuration reading and processing logic #4009

Add package namespace configuration reading and processing logic #4009

Conversation

nkolev92 commented Apr 20, 2021 • edited Loading

Bug

Description

PR Checklist

dominoFire 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

nkolev92 Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

nkolev92 commented Apr 20, 2021

kartheekp-ms 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

nkolev92 commented Apr 21, 2021

erdembayar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominoFire left a comment

Choose a reason for hiding this comment

erdembayar left a comment

Choose a reason for hiding this comment

nkolev92 commented Apr 20, 2021 •

edited

Loading

nkolev92 Apr 21, 2021 •

edited

Loading