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(download): Rename dl() to Invoke-Download() #5143

Merged
merged 10 commits into from
Sep 16, 2022

Conversation

examosa
Copy link
Contributor

@examosa examosa commented Sep 12, 2022

Description

This PR renames the dl and related functions to use more descriptive names that better adhere to PowerShell conventions.

Motivation and Context

Resolves #3830

How Has This Been Tested?

Test suite run on local machine and in CI; all remain passing.

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 refactor: rename dl to Invoke-WebDownload refactor(download): Rename dl to Invoke-WebDownload Sep 13, 2022
@niheaven niheaven changed the title refactor(download): Rename dl to Invoke-WebDownload refactor(download): Rename dl() to Invoke-WebDownload() Sep 13, 2022
@rashil2000
Copy link
Member

I think Invoke-Download is good enough (web is implied)

@examosa
Copy link
Contributor Author

examosa commented Sep 13, 2022

Thanks for the feedback @niheaven. I have a few clarifying questions:

  1. It looks like you're suggesting to remove Invoke-WebDownload and use Invoke-WebRequest directly. Will it be alright to omit the headers from the original code and the -UseBasicParsing flag (PowerShell 5 compatibility)?
  2. Based on the suggested changes to string quotes, would it be alright if I updated the VS Code workspace settings like so?
@@ -4,6 +4,7 @@
     "powershell.codeFormatting.preset": "OTBS",
     "powershell.codeFormatting.alignPropertyValuePairs": true,
     "powershell.codeFormatting.ignoreOneLineBlock": true,
+    "powershell.codeFormatting.useConstantStrings": true,
     "files.exclude": {
         "**/.git": true,
         "**/.svn": true,

@niheaven
Copy link
Member

  1. It looks like you're suggesting to remove Invoke-WebDownload and use Invoke-WebRequest directly. Will it be alright to omit the headers from the original code and the -UseBasicParsing flag (PowerShell 5 compatibility)?

Yes, it should, but in fact this file is no longer used. Now installer script is here in https://github.com/ScoopInstaller/Install.

  1. Based on the suggested changes to string quotes, would it be alright if I updated the VS Code workspace settings like so?
@@ -4,6 +4,7 @@
     "powershell.codeFormatting.preset": "OTBS",
     "powershell.codeFormatting.alignPropertyValuePairs": true,
     "powershell.codeFormatting.ignoreOneLineBlock": true,
+    "powershell.codeFormatting.useConstantStrings": true,
     "files.exclude": {
         "**/.git": true,
         "**/.svn": true,

Yes, it is, and we usually use the following setting too:

"powershell.codeFormatting.useCorrectCasing": true

@examosa examosa changed the title refactor(download): Rename dl() to Invoke-WebDownload() refactor(download): Rename dl() to Invoke-Download() Sep 15, 2022
@examosa examosa requested a review from niheaven September 15, 2022 19:53
@niheaven niheaven merged commit 122fdc1 into ScoopInstaller:develop Sep 16, 2022
@rashil2000
Copy link
Member

@examosa Can you make a corresponding PR to GithubActions too? Otherwise the actions will break when this feature gets released.

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.

3 participants