search

tintoy / msbuild-project-tools-server

Language server for MSBuild intellisense (including PackageReference completion).
MIT License
58 stars 16 forks source link

Add PackageVersion completion #84

Closed RudySchockaert-EngieIT closed 10 months ago

RudySchockaert-EngieIT commented 1 year ago

As of .Net 7 you can opt for Central Package Management. A PackageReference can not have a Version attribute anymore, but the version must be specified on the PackageVersion item. For autocompletion, the behavior for both PackageReference & PackageVersion are the same.

tintoy commented 1 year ago

Thanks for contributing! I’ll take a proper look at this later today 🙂

tintoy commented 12 months ago

Thanks! This enhancement looks good to me, but maybe consider only offering the PackageVersion element completion if the MSBuild project's ManagePackageVersionsCentrally property is set to true? Otherwise people may select it and then wonder why it has no effect. This logic could also be used to determine whether the PackageReference element completion should include a Version attribute.

You might also want to consider renaming the HandlePackageReferenceElementCompletion and HandlePackageReferenceAttributeCompletion methods to HandleElementCompletion and HandleAttributeCompletion since the handler now deals with multiple item types.

RudySchockaert-EngieIT commented 11 months ago

Sorry for the late reply, been a little hectic. I’ll look into your comments and I’ll come back to you.

-- SCHOCKAERT Rudy Expertise User Solutions Digital Workplace GBS IT

@.**@.> T : + 32 2 206 39 18 M: + 32 473 84 69 08

@.***

engie.com

http://www.engie.com/ 36, Boulevard Simon Bolivar (ETB2) Office G10BC

1000 Bruxelles - Belgique From: Adam Friedman @.> Sent: Saturday, 2 December 2023 00:51 To: tintoy/msbuild-project-tools-server @.> Cc: SCHOCKAERT Rudy (ENGIE IT) @.>; Author @.> Subject: Re: [tintoy/msbuild-project-tools-server] Add PackageVersion completion (PR #84)

Thanks! This enhancement looks good to me, but maybe consider only offering the PackageVersion element completion if the MSBuild project's ManagePackageVersionsCentrally property is set to true? Otherwise people may select it and then wonder

Thanks! This enhancement looks good to me, but maybe consider only offering the PackageVersion element completion if the MSBuild project's ManagePackageVersionsCentrally property is set to true? Otherwise people may select it and then wonder why it has no effect.

You might also want to consider renaming the HandlePackageReferenceElementCompletion and HandlePackageReferenceAttributeCompletion methods to HandleElementCompletion and HandleAttributeCompletion since the handler now deals with multiple item types.

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/tintoy/msbuild-project-tools-server/pull/84*issuecomment-1836933273__;Iw!!La4veWw!yw4vInrCj7FvRmqIA-ND1Xq7gGYeeZ1urE2S-2SMKpPZ9EqluUp3NLjREFdRYLVL4FsDE4C9Sw2Ui5lKZ6el8HlDI_pd$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AMRV5YXNTT3ZNAF47ON3U3LYHJUOJAVCNFSM6AAAAAA77ZU4S2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZWHEZTGMRXGM__;!!La4veWw!yw4vInrCj7FvRmqIA-ND1Xq7gGYeeZ1urE2S-2SMKpPZ9EqluUp3NLjREFdRYLVL4FsDE4C9Sw2Ui5lKZ6el8CCSRv8F$. You are receiving this because you authored the thread.Message ID: @.**@.>>

ENGIE Mail Disclaimer: http://www.engie.com/disclaimer/

RudySchockaert-EngieIT commented 11 months ago

I like this proposal. It is easier to read indeed, but just as Adam mentioned already the ‘is’ operator had a Type feeling for me too.

I’m now working on the test setup inluding the vscode extension before I’ll adapt the PR.

-- SCHOCKAERT Rudy Expertise User Solutions Digital Workplace GBS IT

@.**@.> T : + 32 2 206 39 18 M: + 32 473 84 69 08

@.***

engie.com

http://www.engie.com/ 36, Boulevard Simon Bolivar (ETB2) Office G10BC

1000 Bruxelles - Belgique From: DoctorKrolic @.> Sent: Friday, 1 December 2023 08:26 To: tintoy/msbuild-project-tools-server @.> Cc: SCHOCKAERT Rudy (ENGIE IT) @.>; Author @.> Subject: Re: [tintoy/msbuild-project-tools-server] Add PackageVersion completion (PR #84)

@DoctorKrolic commented on this pull request. In src/LanguageServer. Engine/CompletionProviders/ItemMetadataCompletionProvider. cs: > @@ -221,7 +221,7 @@ IEnumerable GetElementCompletions(XmlLocation location, ProjectD

@DoctorKrolic commented on this pull request.

In src/LanguageServer.Engine/CompletionProviders/ItemMetadataCompletionProvider.cshttps://urldefense.com/v3/__https:/github.com/tintoy/msbuild-project-tools-server/pull/84*discussion_r1411713232__;Iw!!La4veWw!xpPeskiSLwwryAbKt01n9edb8WnzFdPG9eF_oiIq1b_fTkox7yXqm3gtB29oXx-JqaplaTEt2hm0Eu06KE5TdzXTH6vJ$:

@@ -221,7 +221,7 @@ IEnumerable GetElementCompletions(XmlLocation location, ProjectD

         HandleTriggerCharacters(triggerCharacters, projectDocument, ref targetRange);

         // These items are handled by PackageReferenceCompletion.

In src/LanguageServer.Engine/ContentProviders/HoverContentProvider.cshttps://urldefense.com/v3/__https:/github.com/tintoy/msbuild-project-tools-server/pull/84*discussion_r1411714008__;Iw!!La4veWw!xpPeskiSLwwryAbKt01n9edb8WnzFdPG9eF_oiIq1b_fTkox7yXqm3gtB29oXx-JqaplaTEt2hm0Eu06KE5Td1RoRJD3$:

@@ -173,7 +173,7 @@ public MarkedStringContainer ItemGroup(MSBuildItemGroup itemGroup)

         if (itemGroup == null)

             throw new ArgumentNullException(nameof(itemGroup));

In src/LanguageServer.Engine/ContentProviders/HoverContentProvider.cshttps://urldefense.com/v3/__https:/github.com/tintoy/msbuild-project-tools-server/pull/84*discussion_r1411714180__;Iw!!La4veWw!xpPeskiSLwwryAbKt01n9edb8WnzFdPG9eF_oiIq1b_fTkox7yXqm3gtB29oXx-JqaplaTEt2hm0Eu06KE5Td2rIyrJ8$:

@@ -363,7 +363,7 @@ public MarkedStringContainer ItemGroupMetadata(MSBuildItemGroup itemGroup, strin

         if (string.IsNullOrWhiteSpace(metadataName))

             throw new ArgumentException("Argument cannot be null, empty, or entirely composed of whitespace: 'metadataName'.", nameof(metadataName));

In src/LanguageServer.Engine/ContentProviders/HoverContentProvider.cshttps://urldefense.com/v3/__https:/github.com/tintoy/msbuild-project-tools-server/pull/84*discussion_r1411714594__;Iw!!La4veWw!xpPeskiSLwwryAbKt01n9edb8WnzFdPG9eF_oiIq1b_fTkox7yXqm3gtB29oXx-JqaplaTEt2hm0Eu06KE5Td_x8F_JT$:

@@ -437,7 +437,7 @@ public MarkedStringContainer UnusedItemGroupMetadata(MSBuildUnusedItemGroup item

         if (string.IsNullOrWhiteSpace(metadataName))

             throw new ArgumentException("Argument cannot be null, empty, or entirely composed of whitespace: 'metadataName'.", nameof(metadataName));

— Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https:/github.com/tintoy/msbuild-project-tools-server/pull/84*pullrequestreview-1759176562__;Iw!!La4veWw!xpPeskiSLwwryAbKt01n9edb8WnzFdPG9eF_oiIq1b_fTkox7yXqm3gtB29oXx-JqaplaTEt2hm0Eu06KE5TdwiaazUZ$, or unsubscribehttps://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AMRV5YQLSKNQ64COLY6XQ4DYHGA77AVCNFSM6AAAAAA77ZU4S2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJZGE3TMNJWGI__;!!La4veWw!xpPeskiSLwwryAbKt01n9edb8WnzFdPG9eF_oiIq1b_fTkox7yXqm3gtB29oXx-JqaplaTEt2hm0Eu06KE5Td2YhXkv4$. You are receiving this because you authored the thread.Message ID: @.**@.>>

ENGIE Mail Disclaimer: http://www.engie.com/disclaimer/

RudySchockaert-EngieIT commented 10 months ago

I added the change as propoised by @DoctorKrolic

tintoy commented 10 months ago

Thanks! I’ll take a look at this first thing tomorrow morning 🙂

tintoy commented 10 months ago

This looks good to me; I'm happy to merge it and then make any additional changes separately, if needed. @DoctorKrolic sound good to you?

tintoy commented 10 months ago

Thanks, @RudySchockaert-EngieIT !