FuGetGallery icon indicating copy to clipboard operation
FuGetGallery copied to clipboard

Added capability to show obsolete members in diff with message

Open jamesmcroft opened this issue 2 years ago • 7 comments

A look at implementing a fix for #167, providing diff information for obsolete members.

Change includes extra NumObsolete fields to the existing diff info types and an ObsoleteMessage field to the MemberDiffInfo type.

Updates made to the diff UI to reflect obsolete members showing the message

jamesmcroft avatar Jun 05 '22 20:06 jamesmcroft

Hi, thank you for that change! This is going to be helpful.

I have just tested the changes and have run into an exception in your code. My assumption is that the code doesn't handle ObsoleteAttribute calls without arguments.

The package MineStat marked multiple methods as obsolete from 1.x->2.0.0, but did not provide workaround messages (only added in 2.1.1). Comparison between 1.x and 2.1.1 work flawlessly, listing the added deprecations without error.

Stacktrace:

InvalidOperationException: Sequence contains no elements
System.Linq.ThrowHelper.ThrowNoElementsException()
   at System.Linq.ThrowHelper.ThrowNoElementsException()
   at System.Linq.Enumerable.First[TSource](IEnumerable`1 source)
   at FuGetGallery.ApiDiff.ValidateObsolete(ICustomAttributeProvider member) in <$ProjectDir>\FuGetGallery\Data\ApiDiff.cs:line 223
   at FuGetGallery.ApiDiff..ctor(PackageData package, PackageTargetFramework framework, PackageData otherPackage, PackageTargetFramework otherFramework) in <$ProjectDir>\FuGetGallery\Data\ApiDiff.cs:line 174
   at FuGetGallery.ApiDiff.ApiDiffCache.<>c__DisplayClass1_0.<GetValueAsync>b__0() in <$ProjectDir>\FuGetGallery\Data\ApiDiff.cs:line 249
   at System.Threading.Tasks.Task`1.InnerInvoke()
   at System.Threading.Tasks.Task.<>c.<.cctor>b__272_0(Object obj)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)

To replicate: call the URL /packages/MineStat/2.0.0/lib/net46/diff/1.0.1/ (Do an API comparison between versions 1.0.1 and 2.0.0 of the package MineStat)

(edit: forgot to include the actual error message 😉)

mindsolve avatar Jun 05 '22 22:06 mindsolve

Thanks for the example, I tried with a few that I'd known and all came out okay 😅

I'll take a look 😄

jamesmcroft avatar Jun 06 '22 05:06 jamesmcroft

@mindsolve that should be solved now. Thanks for testing this 👍🏻

jamesmcroft avatar Jun 06 '22 05:06 jamesmcroft

Does this show obsolete messages in a non-diff context viewing of the API?

TylerBrinkley avatar Jun 13 '22 18:06 TylerBrinkley

@TylerBrinkley: No, it doesn't. I have implemented that feature in my branch based on this PR, so either @jamesmcroft integrates it in here or I'll open a second PR after this one has been merged :) (#173)

mindsolve avatar Jun 13 '22 19:06 mindsolve

@mindsolve @TylerBrinkley happy to look into the non-diff context as part of this change 👍🏻

jamesmcroft avatar Jun 13 '22 19:06 jamesmcroft

Merged the changes in that @mindsolve added.

This PR should now cover API diff obsolete messaging as well as the non-diff context @TylerBrinkley 👍🏻

jamesmcroft avatar Jun 18 '22 12:06 jamesmcroft