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

Cleanup the Polly and Polly.Specs codebase #1290

Open
martintmk opened this issue Jun 14, 2023 · 5 comments
Open

Cleanup the Polly and Polly.Specs codebase #1290

martintmk opened this issue Jun 14, 2023 · 5 comments
Labels
enhancement Hacktoberfest Suggested contribution for Hacktoberfest help wanted A change up for grabs for contributions from the community up-for-grabs

Comments

@martintmk
Copy link
Contributor

In the #1287 we enabled a full set of analyzers for these projects. Many of the rules are currently suppressed in each respective csproj file with the <NoWarn> element.

We can enable these rules one-be-one and cleanup the codebase.

@martincostello martincostello added the help wanted A change up for grabs for contributions from the community label Jun 16, 2023
IgorIgorevich94 added a commit to IgorIgorevich94/Polly that referenced this issue Aug 26, 2023
IgorIgorevich94 added a commit to IgorIgorevich94/Polly that referenced this issue Aug 28, 2023
Co-authored-by: Martin Costello <martin@martincostello.com>
@martincostello martincostello added the Hacktoberfest Suggested contribution for Hacktoberfest label Sep 28, 2023
@davidCett
Copy link

Hi👋I would like to help with this issue

@martincostello
Copy link
Member

@davidCett Thanks for volunteering! Feel free to open a pull request to tackle one of the disabled warnings. I suggest you tackle one or two per pull request to prevent the diffs getting too big to easily review.

@martincostello
Copy link
Member

💡 Tips for future contributors working on this issue:

  • Only the Polly and Polly.Specs projects are still in scope for this issue. Other projects with analyzer suppressions are intentional (we've intentionally turned them off, rather than it being "let's fix it later") and represent cases where we specifically disable a rule for a project because it is not useful or code-level suppressions are too noisy (e.g. the Polly.Benchmarks project disables rules that are not compatible with BenchmarkDotNet's requirements for benchmark classes).
  • Avoid fixing more than one analyzer ID in the same pull request. Unless the changes are trivial, raise a new pull request per analyzer rule - this makes code review much easier for the maintainers as otherwise you could potentially change dozens of files.
  • Consider whether a specific change is useful. If fixing a specific analyzer rule impacts hundreds of files and is a subjective opinion, it may not be worth fixing and can just be left suppressed. If you're unsure, feel free to ask in a comment below before starting the work if you feel it would generate a large change.
  • Do not change the public API in the case of the Polly project. If fixing an analyser rule would require a change to the public API surface (e.g. renaming a member, re-ordering arguments etc., things that require changing a PublicAPI.*.txt file), then do not make the suggested change - instead, suppress the warning(s) on a case-by-case basis with a pair of #pragma warning {disable|restore} {AnalyzerId} directives in the source file(s).
  • If changes add new code to the Polly project (e.g. adding missing validation, rather than style-only changes), then ensure that test coverage is not impacted. This will require adding new unit tests.

@sukreshmanda
Copy link
Contributor

For the warnings xUnit1031, it says blocking calls should not be used in a test method. The below mentioned code uses Task.WaitAll which is a blocking action.

Example :

#pragma warning disable xUnit1031 // Do not use blocking task operations in test method

Task.WaitAll([firstExecution, secondExecution], testTimeoutToExposeDeadlocks).Should().BeTrue();

#pragma warning restore xUnit1031 // Do not use blocking task operations in test method

The essence of the above lines is to check if firstExecution and secondExecution tasks finishes before the specified timeout (testTimeoutToExposeDeadlocks). I want to confirm if the below mentioned non-blocking code can be a proper replacement for the above code.

var allTasksTask = Task.WhenAll(firstExecution, secondExecution);

var completedTask = await Task.WhenAny(allTasksTask, Task.Delay(testTimeoutToExposeDeadlocks));

(completedTask == allTasksTask).Should().BeTrue();

@martincostello
Copy link
Member

If I remember correctly, these tests are specifically testing synchronous code paths, so the suppressions should be left in place.

This was referenced Sep 25, 2024
martincostello pushed a commit that referenced this issue Sep 28, 2024
Contributes to #1290.
@gabidabet gabidabet mentioned this issue Sep 28, 2024
4 tasks
martincostello pushed a commit that referenced this issue Sep 29, 2024
Contributes to #1290.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Hacktoberfest Suggested contribution for Hacktoberfest help wanted A change up for grabs for contributions from the community up-for-grabs
Projects
None yet
Development

No branches or pull requests

4 participants