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

feat(core): Add 'Get-Encoding()' function to fix missing webClient encoding #4956

Merged
merged 5 commits into from
Jun 13, 2022

Conversation

yi-Xu-0100
Copy link
Contributor

@yi-Xu-0100 yi-Xu-0100 commented May 28, 2022

Add Get-Encoding function in core.ps1

Description

Motivation and Context

Closes #4911 #4324

Relates to #4911 #4324

How Has This Been Tested?

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.

yi-Xu-0100 added a commit to yi-Xu-0100/Scoop that referenced this pull request May 28, 2022
@yi-Xu-0100 yi-Xu-0100 changed the title fix(core): use Get-Encoding to set webclient encoding fix(core): add Get-Encoding function to fix missing webClient encoding May 28, 2022
@yi-Xu-0100
Copy link
Contributor Author

@rashil2000 Any review here will be helpful~ 😁

lib/autoupdate.ps1 Outdated Show resolved Hide resolved
lib/core.ps1 Outdated Show resolved Hide resolved
lib/manifest.ps1 Outdated Show resolved Hide resolved
libexec/scoop-virustotal.ps1 Outdated Show resolved Hide resolved
@yi-Xu-0100 yi-Xu-0100 requested a review from niheaven June 3, 2022 09:44
yi-Xu-0100 added a commit to yi-Xu-0100/Scoop that referenced this pull request Jun 3, 2022
@niheaven
Copy link
Member

niheaven commented Jun 4, 2022

Do you have some testing manifests?

@yi-Xu-0100
Copy link
Contributor Author

@niheaven ,Thanks for your reply. 😁

In #4911 (comment) , I use

scoop install "https://raw.githubusercontent.com/ygguorun/scoop-bucket/master/bucket/PDFPatcher.json"

, and this manifest - DoveBoy/Apps/pdfpatcher has the checkver, and they all work well.

@niheaven
Copy link
Member

niheaven commented Jun 6, 2022

checkver has some error, could you please check it again?

@yi-Xu-0100
Copy link
Contributor Author

@niheaven I used it for scoop update and scoop install, they all work well.

Could you please provider the way to test the checkver, I have no idea for it.🤔

@niheaven
Copy link
Member

niheaven commented Jun 9, 2022

Checkout this branch and cd in any bucket, run .\bin\checkver.ps1 xxx, there are some bugs :)

yi-Xu-0100 added a commit to yi-Xu-0100/Scoop that referenced this pull request Jun 9, 2022
…ing (ScoopInstaller#4956)

Add `Get-Encoding` function in core.ps1

closes: ScoopInstaller#4911 ScoopInstaller#4324

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
…ing (ScoopInstaller#4956)

Add `Get-Encoding` function in core.ps1

closes: ScoopInstaller#4911 ScoopInstaller#4324

Co-authored-by: Hsiao-nan Cheung <niheaven@gmail.com>
@yi-Xu-0100
Copy link
Contributor Author

@niheaven Thanks for pointing out the problem, I think it's fixed now. 😁

@niheaven
Copy link
Member

Some tweaks, and please test it @yi-Xu-0100

niheaven
niheaven previously approved these changes Jun 10, 2022
@niheaven niheaven changed the title fix(core): add Get-Encoding function to fix missing webClient encoding feat(core): Add 'Get-Encoding()' function to fix missing webClient encoding Jun 10, 2022
@yi-Xu-0100
Copy link
Contributor Author

@niheaven Thanks for adding the judgment for null values, but there is something we need to revert.

If we use (Get-Encoding($wc)).GetString($wc.DownloadData($url)), then (Get-Encoding($wc)) can only get the return result in UTF-8, because The ResponseHeaders of $wc will be $null before the data is downloaded.

So I re-split the expression to handle the case where charset is something else.

@yi-Xu-0100 yi-Xu-0100 requested a review from niheaven June 11, 2022 02:35
@niheaven
Copy link
Member

Aha, right?

Okay, I'll test it tomorrow and it seems stable enough now.

@niheaven niheaven merged commit 6e25e44 into ScoopInstaller:develop Jun 13, 2022
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.

2 participants