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

Set Invoke-ScriptAnalyzer Path default value #1889

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ThomasNieto
Copy link
Contributor

PR Summary

Removes Path as a mandatory parameter and sets the default to the current working directory.

Resolves #1887

PR Checklist

@bergmeister
Copy link
Collaborator

Nice idea but you'd need to fix existing tests

@ThomasNieto
Copy link
Contributor Author

I'm not sure why the tests are failing with change as the tests would be using a provided path and not a default.

@bergmeister
Copy link
Collaborator

I will try look into it another time and see whether I can do something about it, definitely before next release, sorry it's taken so long

@bergmeister bergmeister marked this pull request as draft January 2, 2024 14:35
@liamjpeters
Copy link
Contributor

liamjpeters commented Mar 27, 2024

@ThomasNieto This seems like a good idea to me 🙂

@bergmeister, This fails many of the tests as the path now always has some value. If -ScriptDefinition is supplied - it's ignored

private bool IsFileParameterSet() => Path is not null;

You should probably make a change to the IsFileParameterSet() function to instead check that ScriptDefinition doesn't have some value. Or maybe better still return this.ParameterSetName.StartsWith("Path").

You will also need to update the markdown help file for Invoke-ScriptAnalyzer to tell the help system that path is no longer required:

Or you'll get a test failure from Tests\Engine\ModuleHelp.Tests.ps1

$codeMandatory = $parameter.IsMandatory.toString()

Hope that helps 😀!

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.

Invoke-ScriptAnalyzer required Path parameter
3 participants