Skip to content

Enable CA18XX rules with suggestion severity #13924

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.

Sign up for GitHub

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

Jump to bottom
Merged
anmenaga merged 1 commit into from
Nov 5, 2020
Merged

Enable CA18XX rules with suggestion severity #13924

anmenaga merged 1 commit into from
Nov 5, 2020

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Oct 29, 2020 •
edited
Loading

Promote CA18XX rules with suggestion severity to warning:

  • CA1821: Remove empty Finalizers
  • CA1824: Mark assemblies with NeutralResourcesLanguageAttribute
  • CA1826: Do not use Enumerable methods on indexable collections
  • CA1828: Do not use CountAsync() or LongCountAsync() when AnyAsync() can be used
  • CA1832: Use AsSpan or AsMemory instead of Range-based indexers when appropriate
  • CA1833: Use AsSpan or AsMemory instead of Range-based indexers when appropriate

Criteria used to select rules for this PR:

  • currently at suggestion severity
  • within the CA18XX range
  • no existing violations.

@ghost ghost assigned anmenaga Oct 29, 2020
@xtqqczze xtqqczze marked this pull request as draft October 29, 2020 16:47
@xtqqczze xtqqczze changed the title Enable CA1828: Do not use CountAsync() or LongCountAsync() when AnyAsync() can be used Enable CA18XX rules with suggestion severity Oct 29, 2020
@xtqqczze xtqqczze force-pushed the CA1828 branch 2 times, most recently from 0054af1 to 5ffa4c4 Compare October 29, 2020 18:16
@xtqqczze
Enable CA18XX rules with suggestion severity
af92178 
* CA1821: Remove empty Finalizers
* CA1824: Mark assemblies with NeutralResourcesLanguageAttribute
* CA1826: Do not use Enumerable methods on indexable collections
* CA1828: Do not use CountAsync() or LongCountAsync() when AnyAsync() can be used
* CA1832: Use AsSpan or AsMemory instead of Range-based indexers when appropriate
* CA1833: Use AsSpan or AsMemory instead of Range-based indexers when appropriate
@xtqqczze xtqqczze marked this pull request as ready for review October 29, 2020 22:59
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 30, 2020

I suppose our goal is to make the existing codebase modern. If there is no code that falls under these rules, we can save time and leave the default values ​​for such rules. Moreover, the default value is "suggestion" so we will see these recommendations anyway.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Oct 30, 2020

I suppose our goal is to make the existing codebase modern. If there is no code that falls under these rules, we can save time and leave the default values ​​for such rules. Moreover, the default value is "suggestion" so we will see these recommendations anyway.

By setting these rules to warning we can ensure regressions do not pass CI.

We cannot rely on suggestion severity because contributors may not be using an IDE with live analysis available. For example, the default in VS Code is for live analysis not to be enabled.

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 30, 2020

I mean today (before next milestone start) we can fix many style and formatting issues with small commits and postpone a rest of the work.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Oct 30, 2020

I mean today (before next milestone start) we can fix many style and formatting issues with small commits and postpone a rest of the work.

@iSazonov OK, I see your point. For future PRs I can focus on style and formatting issues. We may as well merge this PR though.

@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Oct 30, 2020
@anmenaga
Copy link

anmenaga commented Nov 4, 2020

@xtqqczze can you please tell if my understanding is correct:

  1. With severity = warning (unlike severity = suggestion) a rule will generate a build failure?
  2. The fact that CIs for this PR passed means that we currently do Not have violations of rules that are changed by this PR?
  3. Why these specific CA18XX rules were changed in this PR (and not other rules)? What is special about these CA18XX rules so that they were chosen to be updated?

Thank you.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Nov 4, 2020

@anmenaga

  1. With severity = warning (unlike severity = suggestion) a rule will generate a build failure?

Correct, with TreatErrorsAsWarnings enabled (which it is), a rule with warning severity will generate a build failure.

  1. The fact that CIs for this PR passed means that we currently do Not have violations of rules that are changed by this PR?

Correct, there no existing violations of these rules.

  1. Why these specific CA18XX rules were changed in this PR (and not other rules)? What is special about these CA18XX rules so that they were chosen to be updated?

The rules were chosen from the CA18XX range that were already at suggestion severity, and did not generate any build failures. There is nothing special about the CA18XX range other than it is easier to test and submit a PR for a smaller number of rules at a time.

@iSazonov
Copy link
Collaborator

iSazonov commented Nov 4, 2020

What is special about these CA18XX rules so that they were chosen to be updated?

It is a best practice from .Net team and forcing these rules helps us to avoid bad patterns. If there is any edge cases, we can always suppress these rules locally with the directive.

@anmenaga anmenaga merged commit 1c99dcb into PowerShell:master Nov 5, 2020
@iSazonov iSazonov added this to the 7.2.0-preview.1 milestone Nov 5, 2020
@xtqqczze xtqqczze deleted the CA1828 branch November 5, 2020 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

+1 more reviewer

@anmenaga anmenaga anmenaga approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@anmenaga anmenaga

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Milestone

7.2.0-preview.1

Development

Successfully merging this pull request may close these issues.

3 participants

@xtqqczze @iSazonov @anmenaga