Skip to content

Adding Microsoft.CodeAnalysis.BannedApi as suggested for SignalR MessagePackHubProtocol #18290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tebeco opened this issue Jan 11, 2020 · 11 comments
Labels
area-signalr Includes: SignalR clients and servers task

Comments

@tebeco
Copy link
Contributor

tebeco commented Jan 11, 2020

Is your feature request related to a problem? Please describe.

I was trying to do update MessagePack to 2.0.x but as few things changed like the introduction of MessagePackSerializerOptions.
As i forgot to properly instantiate it @AArnott suggested here to use Microsoft.CodeAnalysis.BannedApi :

Use the Microsoft.CodeAnalysis.BannedApiAnalyzers package and add a banned api file to ban all APIs that do not include the options in order to stay on the safe side.
MessagePack-CSharp/MessagePack-CSharp#534 tracks making this easier.

Describe the solution you'd like

Trying to check if such analyzer makes sense and could be used here

Additional context

As i'm submitting this issue, i began to read :
MessagePack-CSharp/MessagePack-CSharp#534
MessagePack-CSharp/MessagePack-CSharp#533
https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeAnalysis.BannedApiAnalyzers/Microsoft.CodeAnalysis.BannedApiAnalyzers.md
https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeAnalysis.BannedApiAnalyzers/BannedApiAnalyzers.Help.md
https://github.com/dotnet/csharplang/blob/master/spec/documentation-comments.md#id-string-format

But i'm not sure how to setup properly this analyzer to be honest.
Especially since the Banned Methods for MessagePack are one with either/both ref and optional parametters so i'm not sure it's going to kicks in properly

@analogrelay
Copy link
Contributor

Given MessagePack-CSharp/MessagePack-CSharp#769, is this just a matter of installing and enabling the new analyzers now? @AArnott are the analyzers available to use?

@tebeco
Copy link
Contributor Author

tebeco commented Mar 18, 2020

that was related :#18291
#18291 (comment)

i'm not sure i understand how it's supposed to work
at that time i understood that there was nothing to do in this repo but i still don't get it ^^

@analogrelay
Copy link
Contributor

I think the idea is that the MessagePack-CSharp project will produce their own set of analyzers. So ideally we would just add a reference to the analyzers they are producing and enable them. But that's why I want to check in with @AArnott.

@AArnott
Copy link
Contributor

AArnott commented Mar 18, 2020

Yes, 2.1.80 of the MessagePackAnalyzer package supports this. Make sure MsgPack001 and MsgPack002 are enabled. The rest can be disabled if you want as they're irrelevant.

@analogrelay
Copy link
Contributor

Cool, so we just need to add a reference and enable the analyzers. Thanks!

@tebeco
Copy link
Contributor Author

tebeco commented Mar 19, 2020

@AArnott
Is the MessagePackAnalyzer will always have the same version of MessagePack ?
I mean, Are they produced in the same build pipeline, and use the exact same logic to extract the version, and does the build always runs for both ?

I'm trying to understand if i should re-use the $(MessagePackPackageVersion) or create a new one

@AArnott
Copy link
Contributor

AArnott commented Mar 19, 2020

Yes, I would reuse the same msbuild property for both versions.

@tebeco
Copy link
Contributor Author

tebeco commented Mar 20, 2020

Should this be closed + milestone associated ?
(Not sure i should close it if there's a merge associated TBH)

@AArnott
Copy link
Contributor

AArnott commented Mar 20, 2020

Closing makes sense to me.

@tebeco
Copy link
Contributor Author

tebeco commented Mar 20, 2020

i wonder if i close it the current milestone needs/will be changed ^^

(cc @anurse )

@BrennanConroy
Copy link
Member

Thanks @tebeco 😄

@ghost ghost locked as resolved and limited conversation to collaborators Apr 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-signalr Includes: SignalR clients and servers task
Projects
None yet
Development

No branches or pull requests

5 participants