Skip to content
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

feat: use Meziantou.Polyfill to allow for nullable static analysis in HttpClientRequestResult #319

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

JMolenkamp
Copy link
Contributor

@JMolenkamp JMolenkamp commented Jun 12, 2024

The library does not contain attributes to be used by the compiler for static nullable analysis.
This results in warnings even though HttpClientRequestResult<TData>.HasData was already evaluated to be true:

image

There are probably more classes where this is useful, but this is where i encountered it.

@perkops
Copy link
Member

perkops commented Jun 13, 2024

Hi @JMolenkamp,

Thank you for the contribution.

However, when running the pipeline, we can see that something in the Polyfill library conflicts with the existing code-base:

https://github.com/atc-net/atc/actions/runs/9488032839/job/26168150710?pr=319

"CSC : error CA1708: Names of 'Types' and 'Helpers, Helpers, Helpers' should differ by more than case (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1708)"

However, we cannot currently pin-point where the error stems from - Sure we can set the warning to none in an .editorconfig, but would not currently be sure what implications this would have.

Do you have any experience with that error when adding the Polyfill library from Meziantou?

@JMolenkamp
Copy link
Contributor Author

JMolenkamp commented Jun 13, 2024

Hi @perkops,

I know of the possibility to declare the attributes for nullable analysis in a code base. While once again searching how to do this, i found this blog post by meziantou and a mention of his Polyfill library. I reckoned this would be easier. Since you were already using a meziantou package, i thought it might be acceptable to add this reference, not knowing it would fail the pipeline.

I do not have any experience with this library yet, but I will try to resolve the issues somewhere in the coming days. According to the github page some customization is possible.

Searching through the Polyfill library (link), four classes named Helpers are found. However, all these classes are marked with the file modifier, so I would not expect them to cause any problems, since they are not accessible from anywhere except for the file they are declared in.

However, at first glance, I cannot find anything named Types.

@perkops perkops self-requested a review June 13, 2024 12:22
@davidkallesen davidkallesen merged commit 56e8646 into atc-net:main Jun 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants