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

Added rule: AvoidLongLines #1329

Merged
merged 16 commits into from
Oct 4, 2019
Merged

Conversation

thomasrayner
Copy link
Contributor

@thomasrayner thomasrayner commented Sep 3, 2019

PR Summary

Addresses #1243. This rule issues a warning for lines that are longer than 120 characters. This default value can be override and the rule is not enabled by default.

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.

Thanks but running this rule by default will break a lot of people.
The rule should be disabled by default and the width should also be configurable. Have a look at rules that inherit from ConfigurableRule for that.

@thomasrayner
Copy link
Contributor Author

@bergmeister I have made the changes you requested :)

@bergmeister
Copy link
Collaborator

The 2 tests need fixing:
The first one is trivial, the expectation can be changed to 59 due to the new rule (maybe we can add some better logic to automatically determine the number of total rules to avoid having to update this test every time...):
[-] get Rules with no parameters supplied 60ms
Expected 58, but got 59.
74: $defaultRules.Count | Should -Be $expectedNumRules
at , C:\projects\psscriptanalyzer\Tests\Engine\GetScriptAnalyzerRule.tests.ps1: line 74
[+] is a positional parameter 14ms
Context When used incorrectly

The second test is for the documentation to have a matching rule document for every rule. Somehow it does not find the PSAvoidLongLines rules. It uses Get-ScriptAnalyzerRule for that, check if the rule gets returned here or maybe it is simply missing the PS prefix in the name?

[-] Every rule must have an entry in the rule documentation README.md file 26ms
Expected $null or empty, but got PSAvoidLongLines.
46: $rulesReadmeDiff | Where-Object SideIndicator -eq "<=" | Foreach-Object InputObject | Should -BeNullOrEmpty
at , C:\projects\psscriptanalyzer\Tests\Documentation\RuleDocumentation.tests.ps1: line 46

@bergmeister
Copy link
Collaborator

bergmeister commented Sep 15, 2019

I've just pushed a fix for the 2 tests and pulled from master. This should make everything green now. Will add some docs and then we should be ready to go.

Rules/AvoidLongLines.cs Outdated Show resolved Hide resolved
Rules/AvoidLongLines.cs Outdated Show resolved Hide resolved
Rules/AvoidLongLines.cs Outdated Show resolved Hide resolved
Rules/AvoidLongLines.cs Outdated Show resolved Hide resolved
Rules/AvoidLongLines.cs Outdated Show resolved Hide resolved
Rules/AvoidLongLines.cs Outdated Show resolved Hide resolved
Rules/Strings.resx Outdated Show resolved Hide resolved
Rules/Strings.resx Outdated Show resolved Hide resolved
RuleDocumentation/AvoidLongLines.md Outdated Show resolved Hide resolved
Tests/Rules/AvoidLongLines.tests.ps1 Show resolved Hide resolved
@bergmeister bergmeister merged commit 2260653 into PowerShell:master Oct 4, 2019
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

3 participants