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

Add the AvoidSemicolonsAsLineTerminators rule to warn about lines ending with a semicolon. Fix (#824) #1806

Merged
merged 2 commits into from
Jul 25, 2022

Conversation

tempora-mutantur
Copy link
Contributor

It's a rule to warn about lines ending with a semicolon

PR Summary

Add the PSAvoidSemicolonsAsLineTerminators rule to warn about lines ending with a semicolon. Covered with tests. The rule is disabled by default as it's more of a styling preference. #824

PR Checklist

Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

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

Looks great to me, I will give it a test drive but I think otherwise it looks ready to me :-) Just going to re-open this PR to trigger the build since it was temporarily disabled at the time you opened the PR,

@sdwheeler
Copy link
Collaborator

@michaeltlombardi Please review. And we need a docs issue opened for this.

Copy link
Contributor

@michaeltlombardi michaeltlombardi left a comment

Choose a reason for hiding this comment

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

Just a few small docs-related items. Thanks so much for this contribution, @tempora-mutantur!

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules
{
/// <summary>
/// AvoidSemicolonsAsLineTerminators: Checks for lines to don't end with semicolons
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor suggestion for grammar and to bring synopsis into same structure as other rules.

Suggested change
/// AvoidSemicolonsAsLineTerminators: Checks for lines to don't end with semicolons
/// AvoidSemicolonsAsLineTerminators: Checks for lines that end with a semicolon.

}

/// <summary>
/// Analyzes the given ast to find violations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest clarifying specific behavior and standardizing to other rules.

Suggested change
/// Analyzes the given ast to find violations.
/// Checks for lines that end with a semicolon.

/// </summary>
/// <param name="ast">AST to be analyzed. This should be non-null</param>
/// <param name="fileName">Name of file that corresponds to the input AST.</param>
/// <returns>A an enumerable type containing the violations</returns>
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor update to standardize to language used by other rules.

Suggested change
/// <returns>A an enumerable type containing the violations</returns>
/// <returns>The diagnostic results of this rule</returns>

}

/// <summary>
/// Retrieves the type of the rule, Builtin, Managed or Module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor fix to standardize casing for valid keys with other items in this doc and to match docs for other rules.

Suggested change
/// Retrieves the type of the rule, Builtin, Managed or Module.
/// Retrieves the type of the rule: builtin, managed, or module.

@@ -465,6 +465,42 @@ internal class Strings {
}
}

/// <summary>
/// Looks up a localized string similar to Avoid long lines.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Looks up a localized string similar to Avoid long lines.
/// Looks up a localized string similar to Avoid semicolons as line terminators.

docs/Rules/AvoidSemicolonsAsLineTerminators.md Outdated Show resolved Hide resolved
It's a rule to warn about lines ending with a semicolon
Copy link
Contributor

@michaeltlombardi michaeltlombardi left a comment

Choose a reason for hiding this comment

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

LGTM! 💜

I filed a docs issue for tracking this on the other side once this PR is merged. 😊

@tempora-mutantur
Copy link
Contributor Author

tempora-mutantur commented Jun 16, 2022

Thank you all.

Could you please advise what I should better do with the PSScriptAnalyzer-CI build failure, got a test failure for WS2022:

Test Directed Graph.Runspaces should be disposed.

Running analyzer 100 times should only create a limited number of runspaces

Expected the actual value to be less than or equal to 14, because Number of Runspaces should be bound (size of runspace pool cache is 10), but got 15.

I could be wrong, but it looks like flakiness.

Copy link
Member

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

this looks great - thanks for the contribution

Engine/Formatter.cs Outdated Show resolved Hide resolved
@bergmeister bergmeister merged commit b881aff into PowerShell:master Jul 25, 2022
@JamesWTruher JamesWTruher added this to the 1.21 milestone Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants