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

Invoke-ScriptAnalyzer required Path parameter #1887

Open
ThomasNieto opened this issue Feb 6, 2023 · 7 comments · May be fixed by #1889
Open

Invoke-ScriptAnalyzer required Path parameter #1887

ThomasNieto opened this issue Feb 6, 2023 · 7 comments · May be fixed by #1889

Comments

@ThomasNieto
Copy link
Contributor

ThomasNieto commented Feb 6, 2023

The required -Path parameter is a usability concern. It should default to . removing the need for the required parameter.

Is there a reason or design choice of why this parameter is required without a default value?

Steps to reproduces

Invoke-ScriptAnalyzer

Expected behavior

# just runs
Invoke-ScriptAnalyzer

Actual behavior

Invoke-ScriptAnalyzer

cmdlet Invoke-ScriptAnalyzer at command pipeline position 1
Supply values for the following parameters:
Path:

If an unexpected error was thrown then please report the full error details using e.g. $error[0] | Select-Object *

Environment data

> $psversiontable

Name                           Value
----                           -----
PSVersion                      7.3.2
PSEdition                      Core
GitCommitId                    7.3.2
OS                             Microsoft Windows 10.0.19044
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

> (Get-Module -ListAvailable PSScriptAnalyzer).Version | ForEach-Object { $_.ToString() }

1.21.0
1.20.0
1.19.1
1.18.3

@bergmeister
Copy link
Collaborator

I don't think there was a particular reason other than ask user to be explicit but I'd be ok with having such a default.

@SydneyhSmith
Copy link
Collaborator

Thanks @ThomasNieto could you please elaborate on the usability concern...this required path is a pattern in PowerShell...it also could be costly to run PSSA on the . path

@ThomasNieto
Copy link
Contributor Author

ThomasNieto commented Feb 14, 2023

@SydneyhSmith because PowerShell best practices is to use a default value over a required parameter when a reasonable default value can be derived from expected use. I would think a common use case for any PowerShell cmdlet and PSSA included is to default to the current working directory.

I don't understand your "costly" statement. Please explain why it is "costly" to run the script analyzer in the current working directory? PSSA defaults to not running recursively. We're not talking about defaulting to the Windows directory with Recurse enabled.

Specifically my use case I ran into is trying to run PSSA in a GitHub action and since GitHub actions current working directory is the GitHub project I would have expected that it would "just work" but it didn't and upon inspecting it was due to this required parameter.

There is no issue from my prospective changing it to be a default value. All existing users have already defined the required path parameter and new users can make use of the default if they so desire.

@ThomasNieto ThomasNieto linked a pull request Feb 14, 2023 that will close this issue
6 tasks
@bergmeister
Copy link
Collaborator

bergmeister commented Feb 15, 2023

I still agree with Thomas and he has good arguments like Recurse not being the default. Can you give examples Sydney where Path is explicitly required but not necessary (Test-Path is not an example as Path parameter is necessary for it)? At first I was sceptical around not being explicit but then I looked at cmdlets like Get-ChildItem or Invoke-Pester that default to current working directory as well

@ghost
Copy link

ghost commented Mar 2, 2023

Closing due to inactivity

@ghost ghost closed this as completed Mar 2, 2023
@ThomasNieto
Copy link
Contributor Author

@bergmeister / @SydneyhSmith why was this closed?

@bergmeister
Copy link
Collaborator

The bot did it because of the 'Need repro info' tag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants