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

Python: Use install script instead of original installer #6

Merged
merged 5 commits into from
Apr 30, 2019

Conversation

niheaven
Copy link
Member

@niheaven niheaven commented Apr 23, 2019

Copy link
Contributor

@Ash258 Ash258 left a comment

Choose a reason for hiding this comment

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

Add closing directive for PRs inside core bucket

@niheaven niheaven requested a review from Ash258 April 24, 2019 02:40
@niheaven
Copy link
Member Author

niheaven commented Apr 25, 2019

@Ash258 I try to add space in dir name, and it pass through $path correctly. Do we misunderstand something?

If pass through a string to Invoke-Expression (likes https://github.com/lukesampson/scoop/blob/master/lib/install.ps1#L739), it will not expand variables in the string, so white spaces will pass through, and adding double quotes besides these variables is in fact unnecessary。

white-space

@niheaven
Copy link
Member Author

And for FullName, since Get-ChildItem return System.IO.FileSystemInfo, and this type is pass to inner function, if needed, fullname is used.

fullname

@Ash258
Copy link
Contributor

Ash258 commented Apr 25, 2019

Final word on the fullnames. Here is default powershell (windows 10, right) and latest pwsh.
There you can see different behaviour. Same folder, same system, different output.

image

PowerShell/PowerShell#7084

Just use Select-Object -Property Fullname or (..).FullName and it will be safe for every environment

@niheaven
Copy link
Member Author

Yes, for safety, FullName is better.

@niheaven
Copy link
Member Author

What about double quotes? Invoke-Expression with param as [String] needn't double quotes, I'm sure.

@Ash258
Copy link
Contributor

Ash258 commented Apr 25, 2019

I will test lately today with clean W10. Now I will be busy with work stuff for few next hours.

Copy link
Contributor

@Ash258 Ash258 left a comment

Choose a reason for hiding this comment

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

Also adopt new function naming

@niheaven niheaven requested a review from Ash258 April 30, 2019 12:34
"version": "3.7.3",
"depends": "dark",
Copy link
Contributor

Choose a reason for hiding this comment

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

We could open discussion if prefixing main bucket requirements is suitable. When i am thinking about this, i do not know how current implementation will handle multiple manifests with same name.

For Examle main/dark, 'Ash258/dark' If dark is in both buckets, mine will be used (alphabetic order, as far my testing with simple scoop install Wavebox, Ash258 bucket is preferred over extras, which contains Wavebox too)

Copy link
Contributor

@Ash258 Ash258 left a comment

Choose a reason for hiding this comment

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

LGTM

@r15ch13 r15ch13 merged commit b8fa7fd into ScoopInstaller:master Apr 30, 2019
@niheaven niheaven deleted the fix-python branch April 30, 2019 15:16
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.

Issue with python package
3 participants