Add nfproj to supported project extensions #3943

josesimoes · 2021-03-08T11:00:05Z

Bug

Fixes: NuGet/Home#10562.

Regression? Last working version:

Description

Add extension to supported projects.
Add unit tests for install and update commands in CLI.

PR Checklist

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

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

donnie-msft

Hello! My first pass is just pointing out obvious coding guideline violations. See rule #10.
Will take another look soon...

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

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

src/NuGet.Clients/NuGet.CommandLine/Common/ProjectHelper.cs

donnie-msft · 2021-03-09T03:42:35Z

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

+            slnContent.AppendLine("# Visual Studio Version 16");
+            slnContent.AppendLine("VisualStudioVersion = 16.0.31005.135");
+
+            foreach(string project in projectList)


space after foreach

no more space

Please check below example, we want space foreach ( :

NuGet.Client/src/NuGet.Clients/NuGet.CommandLine/Commands/ProjectFactory.cs

Lines 969 to 974 in 73e5977

foreach (var reader in packagesAndDependencies)

{

foreach (var set in reader.Item1.GetPackageDependencies())

{

foreach (var dependency in set.Packages)

{

Another way to deal with syntax is install dotnet-format tool from https://github.com/dotnet/format
then run dotnet-format --exclude submodules, it'll take care of it for you.

Got you. Should be OK now

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

erdembayar · 2021-03-24T20:12:18Z

@josesimoes
Hi. There're some unaddressed PR comments and conflict with dev branch.
Please rebase to dev branch and address PR comments.
If you're not able to do it in anytime soon then could you please change PR to draft mode?

josesimoes · 2021-03-24T22:27:51Z

@erdembayar rebase done!

As for the unaddressed comments, I couldn't find none... Can you please point me to those?

erdembayar · 2021-03-25T18:39:25Z

@erdembayar rebase done!

As for the unaddressed comments, I couldn't find none... Can you please point me to those?

I can see one unaddressed here: #3943 (comment)
Also why did you mark as resolved https://github.com/NuGet/NuGet.Client/pull/3943/files#r589666134 ? Can give some context about meeting or proposal you mentioned here?

josesimoes · 2021-03-25T18:54:41Z

Also why did you mark as resolved https://github.com/NuGet/NuGet.Client/pull/3943/files#r589666134 ?

Just unresolved it.

Can give some context about meeting or proposal you mentioned here?
Sure: it was discussed the creation of a TFM for .NET nanoFramework.
You can check with @JonDouglas or @terrajobst or @clairernovotny although I don't think it's relevant for the PR here...

erdembayar

@josesimoes
Thank you for your contribution. There are still some work need to do.
Please review PR comment and clean up. There are some unused items.

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

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

erdembayar · 2021-03-29T16:56:07Z

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

+        /// Create a simple package with a lib folder for .NET nanoFramework
+        /// The package will be removed from the machine cache upon creation
+        /// </summary>
+        public static ZipPackage CreateNFPackage(string repositoryPath, string id, string version)


What is the use of this one? Not used.

erdembayar · 2021-03-29T16:57:59Z

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

+        /// <param name="path">Path where the package will be saved</param>
+        /// <param name="contentFiles">Content file(s) to be added to the package</param>
+        /// <returns></returns>
+        public static string CreateNFTestPackage(


Here nothing special about NF is going on. How about using existing create package utility instead of creating new one?

Because we don't have a TFM, our DLLs have to be placed inside the lib folder. I couldn't find a nice way to do it with the existing methods. If there is one, please point me to it.

@nkolev92
Do you have any recommendation here?

Look into SimpleTestPackageContext and then do Files.Clear() and add a few file paths,

NuGet.Client/test/TestUtilities/Test.Utility/SimpleTestSetup/SimpleTestPackageContext.cs

Line 52 in c6ca6eb

public List<KeyValuePair<string, byte[]>> Files { get; set; } = new List<KeyValuePair<string, byte[]>>();

.

If you search for references of Files, you'll likely find many tests already do that.

I believe I've replaced them all with the equivalent ones from SimpleTestPackage.

@josesimoes
I assume you're going to remove this one since it's not referenced anymore.

erdembayar · 2021-03-29T16:59:17Z

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

@@ -1841,5 +1841,43 @@ public static CommandRunnerResult RunInstall(SimpleTestPathContext pathContext,

            return r;
        }
+
+        [Fact]
+        public void InstallCommand_NF_Project()


Please check other comment about CreateNFTestPackage.
Here nothing is actually special about 'NF'.

Because of the above, I thought it was relevant to have this one here. If it's not please confirm and I'll remove it.

Currently I don't see any value in this, so I prefer to remove it.

OK. Removing it.

Ok. I'll review again. Also could you be able to make incremental commits instead quashing to 1 commit next time?
It's hard to see what was actual change from last time. I have to start again.

oh... I apologize... though you would prefer it as a single commit...

We squash when merging in the dev branch for this exact reason.

Understood. Each project handles this differently... there was nothing about this on the contributing guide. I'll start doing that.

Honestly, it's something we haven't documented.

The incremental commits has been largely at the discretion of the contributor + reviewers.

erdembayar

@josesimoes
We're getting very close, just 1 more comment from me.
Can you address comment from nkolev92?

erdembayar · 2021-03-31T00:33:03Z

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

@@ -1841,5 +1841,43 @@ public static CommandRunnerResult RunInstall(SimpleTestPathContext pathContext,

            return r;
        }
+
+        [Fact]
+        public void InstallCommand_NF_Project()


Currently I don't see any value in this, so I prefer to remove it.

erdembayar · 2021-03-31T00:35:00Z

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

+        /// <param name="path">Path where the package will be saved</param>
+        /// <param name="contentFiles">Content file(s) to be added to the package</param>
+        /// <returns></returns>
+        public static string CreateNFTestPackage(


@nkolev92
Do you have any recommendation here?

nkolev92 · 2021-03-31T18:26:54Z

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

@@ -1237,5 +1237,155 @@ public static void TestCommandInvalidArguments(string command)
            // Verify traits of help message in stdout
            Assert.Contains("usage:", result.Item2);
        }
+
+        #region helper methods for .NET nanoFramework tests


Rule #16 https://github.com/NuGet/NuGet.Client/blob/dev/docs/coding-guidelines.md

erdembayar

@josesimoes
Good job.
Please remove unused method, then we're good.
Also just in case rebase with latest dev branch. Once it passes CI build then I'll approve.

erdembayar · 2021-04-05T19:32:54Z

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

+        /// <param name="path">Path where the package will be saved</param>
+        /// <param name="contentFiles">Content file(s) to be added to the package</param>
+        /// <returns></returns>
+        public static string CreateNFTestPackage(


@josesimoes
I assume you're going to remove this one since it's not referenced anymore.

erdembayar · 2021-04-05T19:34:02Z

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

+                a1Package.Files.Clear();
+                a1Package.AddFile($"lib/{a1.Id}.dll");
+
+                var a1File = await a1Package.CreateAsFileAsync(packagesSourceDirectory, a1Package.PackageName);


Replace var with FileInfo type.

erdembayar · 2021-04-05T19:34:21Z

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

+                b1Package.Files.Clear();
+                b1Package.AddFile($"lib/{b1.Id}.dll");
+
+                var b1File = await a1Package.CreateAsFileAsync(packagesSourceDirectory, b1Package.PackageName);


Replace var with FileInfo type.

- Add extension to supported projects. - Add unit tests for install and update commands in CLI.

josesimoes · 2021-04-05T23:29:41Z

Good job. 👍🏻

Please remove unused method, then we're good.

Done

Also just in case rebase with latest dev branch. Once it passes CI build then I'll approve.

Rebased

josesimoes · 2021-04-06T18:35:45Z

Anything I need to do/change to address whatever is causing those checks to fail?

erdembayar · 2021-04-06T19:18:54Z

Anything I need to do/change to address whatever is causing those checks to fail?

Most likely no, I'll take care of it.

erdembayar

@josesimoes
Thank you for contribution.
It'll merge today or tomorrow.

josesimoes requested a review from a team as a code owner March 8, 2021 11:00

erdembayar reviewed Mar 8, 2021

View reviewed changes

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetUpdateCommandTests.cs Outdated Show resolved Hide resolved

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/Util.cs Show resolved Hide resolved

donnie-msft reviewed Mar 8, 2021

View reviewed changes

donnie-msft added the Community PRs created by someone not in the NuGet team label Mar 8, 2021

josesimoes force-pushed the work-10562 branch from c700bb5 to 8becb44 Compare March 8, 2021 18:42

nkolev92 reviewed Mar 8, 2021

View reviewed changes

src/NuGet.Clients/NuGet.CommandLine/Common/ProjectHelper.cs Show resolved Hide resolved

donnie-msft reviewed Mar 9, 2021

View reviewed changes

test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetUpdateCommandTests.cs Outdated Show resolved Hide resolved

josesimoes force-pushed the work-10562 branch from 8becb44 to 8113c89 Compare March 9, 2021 07:04

zivkan assigned erdembayar Mar 15, 2021

josesimoes force-pushed the work-10562 branch from 8113c89 to 9eaa865 Compare March 24, 2021 22:27

josesimoes force-pushed the work-10562 branch from 9eaa865 to 219446e Compare March 26, 2021 22:37

erdembayar added the Approved for CI label Mar 26, 2021

erdembayar reviewed Mar 29, 2021

View reviewed changes

erdembayar removed the Approved for CI label Mar 29, 2021

josesimoes force-pushed the work-10562 branch 4 times, most recently from 52435b4 to d123a0c Compare March 30, 2021 07:06

erdembayar reviewed Mar 31, 2021

View reviewed changes

josesimoes force-pushed the work-10562 branch from d123a0c to d4551ab Compare March 31, 2021 03:22

erdembayar added the Approved for CI label Mar 31, 2021

nkolev92 reviewed Mar 31, 2021

View reviewed changes

erdembayar added Approved for CI and removed Approved for CI labels Apr 5, 2021

erdembayar reviewed Apr 5, 2021

View reviewed changes

josesimoes added 4 commits April 6, 2021 00:26

Add nfproj to supported project extensions

7817df9

- Add extension to supported projects. - Add unit tests for install and update commands in CLI.

Remove region from utils

a6733fa

Replace with FileInfo

a451328

Remove unused method

169af33

josesimoes force-pushed the work-10562 branch from 87bb0f8 to 169af33 Compare April 5, 2021 23:28

erdembayar added Approved for CI and removed Approved for CI labels Apr 6, 2021

nkolev92 approved these changes Apr 6, 2021

View reviewed changes

erdembayar approved these changes Apr 6, 2021

View reviewed changes

erdembayar merged commit 22e7f91 into NuGet:dev Apr 7, 2021

josesimoes deleted the work-10562 branch April 7, 2021 06:12

josesimoes mentioned this pull request Nov 10, 2021

Add TFM for .NET nanoFramework NuGet/Home#10800

Closed

josesimoes mentioned this pull request Oct 27, 2022

[Feature]: Add framework badge for .NET nanoFramework NuGet/NuGetGallery#9286

Open

	foreach (var reader in packagesAndDependencies)
	{
	foreach (var set in reader.Item1.GetPackageDependencies())
	{
	foreach (var dependency in set.Packages)
	{

Add nfproj to supported project extensions #3943

Add nfproj to supported project extensions #3943

Conversation

josesimoes commented Mar 8, 2021 • edited Loading

Bug

Description

PR Checklist

donnie-msft 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

erdembayar commented Mar 24, 2021

josesimoes commented Mar 24, 2021

erdembayar commented Mar 25, 2021 • edited Loading

josesimoes commented Mar 25, 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

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

erdembayar Mar 31, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josesimoes commented Apr 5, 2021

josesimoes commented Apr 6, 2021

erdembayar commented Apr 6, 2021

erdembayar left a comment

Choose a reason for hiding this comment

josesimoes commented Mar 8, 2021 •

edited

Loading

erdembayar commented Mar 25, 2021 •

edited

Loading

erdembayar Mar 31, 2021 •

edited

Loading