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

Fix S3872 warning #2111

Merged
merged 4 commits into from
May 15, 2024
Merged

Conversation

iamdmitrij
Copy link
Contributor

@iamdmitrij iamdmitrij commented May 15, 2024

Pull Request

The issue or feature being addressed

#1290

Details on the issue fix or feature implementation

  • Suppress S3872 in the code

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

@iamdmitrij iamdmitrij closed this May 15, 2024
@iamdmitrij iamdmitrij reopened this May 15, 2024
@iamdmitrij iamdmitrij changed the title Fix S3872 Fix S3872 warning May 15, 2024
Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't accept any change that requires you touching the PublicAPI.Shipped.txt files, as those would be breaking changes.

You would need to suppress any warnings generated instead.

@iamdmitrij iamdmitrij reopened this May 15, 2024
@iamdmitrij
Copy link
Contributor Author

Ok, I've disabled this rule only for impacted classes in .editoconfig. If new major versions gets released, it'd be good idea to fix it permanently.

.editorconfig Outdated
@@ -18,6 +18,10 @@ indent_style = space
indent_size = 2
tab_width = 2

# S3872 // Interfaces should not be empty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't appear to be the right description for what the original change was?

It's better (IMHO) to suppress these in the code with #pragma warning disable/restore pairs at the specific points of usage, rather the hiding it away in the editorconfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, in this case we don't have many violations for this rule, so it makes sense to keep it in code. Also, it can serve as a reminder to fix it in the future.

@@ -109,7 +109,9 @@ public static TimeoutPolicy Timeout(int seconds, TimeoutStrategy timeoutStrategy
/// <param name="timeout">The timeout.</param>
/// <returns>The policy instance.</returns>
/// <exception cref="ArgumentOutOfRangeException">timeout;Value must be a positive TimeSpan (or Timeout.InfiniteTimeSpan to indicate no timeout).</exception>
#pragma warning disable S3872
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build failures are annoying - I've seen them before where dotnet format doesn't complain locally but does in CI. I think to fix them you need to move the disable to above the /// comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, works like a charm now.

@martincostello martincostello merged commit 261129b into App-vNext:main May 15, 2024
16 checks passed
@martincostello
Copy link
Member

Thanks again!

@iamdmitrij iamdmitrij mentioned this pull request May 16, 2024
5 tasks
@iamdmitrij iamdmitrij deleted the rename-timeout-param branch May 16, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants