Skip to content

Make -Path a required parameter for Save-PSResource cmdlet #780

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

Merged
merged 2 commits into from
Aug 31, 2022
Merged

Make -Path a required parameter for Save-PSResource cmdlet #780

merged 2 commits into from
Aug 31, 2022

Conversation

alyssa1303
Copy link
Contributor

@alyssa1303 alyssa1303 commented Aug 30, 2022

PR Summary

This PR makes -Path a required parameter for Save-PSResource cmdlet and changes its logic.

PR Context

PR Checklist

@anamnavi anamnavi changed the title Make -Path a required parameter Make -Path a required parameter for Save-PSResource cmdlet Aug 30, 2022
Copy link
Member

@anamnavi anamnavi left a comment

Choose a reason for hiding this comment

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

LGTM

else {
resolvedPath = SessionState.Path.GetResolvedPSPathFromPSPath(value).First().Path;
}
if (WildcardPattern.ContainsWildcardCharacters(value))
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of checking for wildcards here we should try to resolve the path and if that throws, catch and display the error.

Copy link
Member

Choose a reason for hiding this comment

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

That way if there's any issues at all with the path we can properly handle and emit the error.

Copy link
Contributor

@PaulHigin PaulHigin Aug 30, 2022

Choose a reason for hiding this comment

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

The problem with wildcards in the path is that they can return multiple matches, and then which path do we use? Whatever we choose will not be apparent to the user, unless we write out a message.

I feel we should not support wildcards in this case, and require the user to provide a single path that resolves to a real location. If not then that is a terminating error. We don't need to test the path and return an error, because GetResolvedPSPathFromPSPath already does this and throws an exception with a readable error:

"Cannot find path 'C:\temp\temp' because it does not exist."

Copy link
Member

Choose a reason for hiding this comment

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

That's true, this makes sense

Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

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

LGTM

else {
resolvedPath = SessionState.Path.GetResolvedPSPathFromPSPath(value).First().Path;
}
if (WildcardPattern.ContainsWildcardCharacters(value))
Copy link
Contributor

@PaulHigin PaulHigin Aug 30, 2022

Choose a reason for hiding this comment

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

The problem with wildcards in the path is that they can return multiple matches, and then which path do we use? Whatever we choose will not be apparent to the user, unless we write out a message.

I feel we should not support wildcards in this case, and require the user to provide a single path that resolves to a real location. If not then that is a terminating error. We don't need to test the path and return an error, because GetResolvedPSPathFromPSPath already does this and throws an exception with a readable error:

"Cannot find path 'C:\temp\temp' because it does not exist."

@alyssa1303 alyssa1303 merged commit fdc1c01 into PowerShell:master Aug 31, 2022
anamnavi pushed a commit that referenced this pull request Sep 7, 2022
Make -Path a required parameter for Save-PSResource cmdlet
@alyssa1303 alyssa1303 added PR-Bug This pull request resolves an issue bug Release This pull request is part the next release labels Sep 22, 2022
@alyssa1303 alyssa1303 removed the Release This pull request is part the next release label Nov 2, 2022
@SydneyhSmith SydneyhSmith removed the PR-Bug This pull request resolves an issue bug label May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants