GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Enable CA18XX rules with suggestion severity by xtqqczze · Pull Request #13924 · PowerShell/PowerShell

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Oct 29, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ghost ghost assigned Andrew (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 force-pushed the CA1828 branch from 5ffa4c4 to af92178 Compare October 29, 2020 22:15
@xtqqczze
xtqqczze marked this pull request as ready for review October 29, 2020 22:59
@iSazonov

Ilya (iSazonov) commented Oct 30, 2020

Copy link
Copy Markdown
Collaborator

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@xtqqczze

xtqqczze commented Oct 30, 2020

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Oct 30, 2020

Copy link
Copy Markdown
Collaborator

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@xtqqczze

xtqqczze commented Oct 30, 2020

Copy link
Copy Markdown
Contributor Author

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.

Ilya (@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov Ilya (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
iSazonov
Ilya (iSazonov) approved these changes Oct 30, 2020
View reviewed changes
@anmenaga

Andrew (anmenaga) commented Nov 4, 2020

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@xtqqczze

xtqqczze commented Nov 4, 2020

Copy link
Copy Markdown
Contributor Author

Andrew (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Nov 4, 2020

Copy link
Copy Markdown
Collaborator

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anmenaga
Andrew (anmenaga) approved these changes Nov 5, 2020
View reviewed changes
@anmenaga
Andrew (anmenaga) merged commit 1c99dcb into PowerShell:master Nov 5, 2020
@iSazonov Ilya (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
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@xtqqczze
Enable CA18XX rules with suggestion severity (PowerShell#13924)
587490e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov Ilya (iSazonov) iSazonov approved these changes
+1 more reviewer
@anmenaga Andrew (anmenaga) anmenaga approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@anmenaga Andrew (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.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@xtqqczze @iSazonov @anmenaga
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.