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

Allow -Setting parameter to resolve setting presets as well when object is still a PSObject in BeginProcessing #928

Merged

Conversation

bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Mar 11, 2018

PR Summary

Fixes issue #807

Similar to the recent improvement to allow parsing the -Setting object as string when the object is still a PSObject also take into account setting presets like e.g. 'CodeFormattingStroustrup'.
Existing code was cleaned up to reduce duplication.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • User facing documentation needed
  • Change is not breaking
  • Make sure you've added a new test if existing tests do not effectively test the code changed.
  • This PR is ready to merge and is not work in progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

…ject is not resolved to a string yet. Tidy up existing logic.

TODO: add test (we do not have test coverage for setting presets)
@bergmeister bergmeister self-assigned this Mar 11, 2018
@bergmeister bergmeister added this to the 1.17 milestone Mar 11, 2018
@bergmeister bergmeister changed the title WIP (CI test needed): Allow -Setting parameter to resolve setting presets as well when object is still a PSObject in BeginProcessing Allow -Setting parameter to resolve setting presets as well when object is still a PSObject in BeginProcessing Mar 11, 2018
@@ -406,6 +406,13 @@ Describe "Test CustomizedRulePath" {
Pop-Location
}
}

It "resolves rule preset when passed in via pipeline" {
$warnings = 'CodeFormattingStroustrup' | ForEach-Object {
Copy link
Member

Choose a reason for hiding this comment

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

this seems overly complicated, isn't this just:

$warnings = Invoke-ScriptAnalyzer -ScriptDefinition 'if ($true){}' -Settings CodeFormattingStroustrup
$warnings.Count | Should -Be 1
$warnings.RuleName | Should -Be 'PSUseConsistentWhitespace'

all the pipes and where's aren't needed and only add complexity

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The pipes are needed to reproduce the actual bug because otherwise the settings object is already resolved to a string in your case but I will get rid of the where clause and add the additional assertion as you suggested.

settingsMode = SettingsMode.File;
}
}
TryResolveSettingForStringType(settingsFoundPSObject.BaseObject, ref settingsMode, ref settingsFound);
Copy link
Member

Choose a reason for hiding this comment

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

there's no check of the return, does it not matter if we get to this point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The check for the return value is only needed for the first method call. If we still cannot resolve the settings type at this point, then the mode will be SettingsMode.None (this is the initial default) and that gets handled later on.

@bergmeister bergmeister merged commit d1bec74 into PowerShell:development Mar 28, 2018
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.

2 participants