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

refactor(path): Use 'Convert-Path()' instead of 'Resolve-Path()' #5109

Merged
merged 3 commits into from
Aug 22, 2022

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Aug 15, 2022

Description

Use Convert-Path() instead of Resolve-Path() while the latter will resolve the wildcard characters in a path (we just want to convert a path from a PowerShell path to a PowerShell provider path or expand a path)

Motivation and Context

None

How Has This Been Tested?

Internal tests passed, all functions should work well, and more tests should be made.

Checklist:

  • I have read the Contributing Guide.
  • I have ensured that I am targeting the develop branch.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.
  • I have added an entry in the CHANGELOG.

@niheaven niheaven changed the title fix(path): Use 'Convert-Path()' instead of 'Resolve-Path()' refactor(path): Use 'Convert-Path()' instead of 'Resolve-Path()' Aug 15, 2022
@niheaven
Copy link
Member Author

Any comments here, guys?

@rashil2000
Copy link
Member

Are there any bugs we're facing with Resolve-Path? I don't see a reason to change what's already working.

@niheaven
Copy link
Member Author

niheaven commented Aug 21, 2022

As what I said, Resolve-Path is used to RESOLVE or GUESS the path, such as Resolve-Path windows, just see its examples here. What we used in Scoop is just Convert-Path, e.g. expand . or ~.

And there is another benefit here, Resolve-Path outputs [System.Management.Automation.PathInfo] which may be handled incorrectly while Convert-Path outputs [System.String].

I don't see a reason to change what's already working.

So it is a 'refactor'

@niheaven niheaven merged commit 1985a05 into develop Aug 22, 2022
@niheaven niheaven deleted the use_convert-path branch August 22, 2022 07:12
@Hrxn
Copy link

Hrxn commented Oct 14, 2022

As what I said, Resolve-Path is used to RESOLVE or GUESS the path, such as Resolve-Path windows, just see its examples here. What we used in Scoop is just Convert-Path, e.g. expand . or ~.

Not really. That example is specific to $PWD = C:\
Resolve-Path does not "guess" any paths..

And there is another benefit here, Resolve-Path outputs [System.Management.Automation.PathInfo] which may be handled incorrectly while Convert-Path outputs [System.String].

Yeah, that's the one important difference between these two cmdlets.
One could argue that using PathInfo objects would be beneficial over using just strings, but this depends on the use case.
If you're dealing with paths represented as strings anyway, then Convert-Path is just fine, yes..

The other important difference is that Resolve-Path does support using PSCredential for credentialed access, but this is probably not relevant for scoop's case..

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.

None yet

3 participants