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

Rename params: *url to *uri #551

Merged
merged 8 commits into from Feb 3, 2022
Merged

Conversation

fsackur
Copy link
Contributor

@fsackur fsackur commented Nov 20, 2021

PR Summary

  • Changed -URL params to -Uri (including, e.g., -LicenseUrl => -LicenseUri)
    • Updated (almost) all identifiers in source code url => uri, for consistency
    • Updated docs
    • A few strings and comments left at URL, where they make sense in the context
  • Added test/result* to .gitignore
  • Amended build.ps1 to try importing PSPackageProject before installing again

PR Context

Ref #460

Broadly speaking, all installed-by-default modules use uri in parameter names instead of url. To provide consistency, I think PowerShellGet should also follow this convention.

PR Checklist

@fsackur fsackur marked this pull request as ready for review Nov 22, 2021
@fsackur
Copy link
Contributor Author

@fsackur fsackur commented Nov 22, 2021

Requesting feedback!

I hope it's not too contentious to change all the terminology in private code; it seemed to me that leaving inconsistencies would be a poor dev experience for future maintainers.

@fsackur fsackur changed the title WIP: Rename params: *url to *uri Rename params: *url to *uri Nov 22, 2021
Copy link
Member

@alerickson alerickson left a comment

Thank you @fsackur for opening up this PR!
Just a few things--

  1. There's a handful of changes in PublishPSResource that should actually not be changed, specifically 'licenseUrl', 'iconUrl', and 'projectUrl'. Those names are specified in the module manifest, which is used to generate the nuspec, so it'll cause problems when publishing (see nuspec reference if interested)

  2. This is very minor but the last two tests (SetPSResourceRepository.Tests.ps1 and UnregisterPSResourceRepository.Tests.ps1) have Uri in all caps whereas the other tests have it in pascal case. I would recommend changing the former tests to pascal case just to be consistent. Of course this is just an aesthetic request!

src/code/PublishPSResource.cs Outdated Show resolved Hide resolved
test/PublishPSResource.Tests.ps1 Outdated Show resolved Hide resolved
test/PublishPSResource.Tests.ps1 Outdated Show resolved Hide resolved
test/PublishPSResource.Tests.ps1 Outdated Show resolved Hide resolved
test/PublishPSResource.Tests.ps1 Outdated Show resolved Hide resolved
src/code/PublishPSResource.cs Outdated Show resolved Hide resolved
src/code/PublishPSResource.cs Outdated Show resolved Hide resolved
src/code/PublishPSResource.cs Outdated Show resolved Hide resolved
src/code/PublishPSResource.cs Outdated Show resolved Hide resolved
src/code/PublishPSResource.cs Outdated Show resolved Hide resolved
Copy link
Member

@anamnavi anamnavi left a comment

Hi @fsackur! Thanks for creating this PR and for diligently going through all the files and making appropriate changes. I know it looks like a lot of comments but I had feedback in 3 main areas:

  1. please remove the test/result* file reference from the .gitignore. I imagine it's for your local file purposes. Thanks :)

  2. it's a bit of a nitpick but it would be nice to match the casing of cmdlet's Uri parameter across all tests/code. I know in some of the original code we accidentally had it as "URL" so I understand changing it to "URI" however "Uri" where appropriate would be best. I left suggestions on where all this would happen.

  3. I echoed @alerickson's feedback on reverting the *url to *uri change for LicenseUrl, IconUrl, ProjectUrl in a few documentation files just so that we don't miss it.

Thanks for making all these changes, it's looking great :D

Docs/PublishPSResource.md Outdated Show resolved Hide resolved
Docs/RegisterPSResourceRepository.md Outdated Show resolved Hide resolved
Docs/UnregisterPSResourceRepository.md Outdated Show resolved Hide resolved
src/code/PSRepositoryInfo.cs Outdated Show resolved Hide resolved
src/code/FindHelper.cs Show resolved Hide resolved
test/SetPSResourceRepository.Tests.ps1 Outdated Show resolved Hide resolved
test/SetPSResourceRepository.Tests.ps1 Outdated Show resolved Hide resolved
test/UnregisterPSResourceRepository.Tests.ps1 Outdated Show resolved Hide resolved
test/RegisterPSResourceRepository.Tests.ps1 Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@fsackur
Copy link
Contributor Author

@fsackur fsackur commented Nov 29, 2021

My thanks for the detail - this represents a lot of review work. I will not be able to look at it for 10 days at least - I aim to update the PR by 2021-12-16.

@fsackur
Copy link
Contributor Author

@fsackur fsackur commented Jan 24, 2022

Aplogies for sitting on this so long, after anamnavi put so much time into review. I will update the PR this week

@kilasuit
Copy link

@kilasuit kilasuit commented Jan 24, 2022

@alerickson / @anamnavi - It's worth remembering that whilst nuget uses ProjectURL & LicenseURL, PowerShell Module Manifests have always used ProjectUri & LicenseUri as their properties as can be seen in the New-ModuleManifest doc

Whilst I haven't looked deeply at this repositories code base (or this PR either) we need to be sure that the module manifest is correct as people who run the following would end up being broken if the resultant manifest uses Url not Uri in all the properties in the PSData hashtable under the PrivateData HashTable.

Just thought it worth mentioning

@fsackur fsackur force-pushed the rename-param-url-to-uri branch from 785754a to 6259add Compare Jan 30, 2022
@fsackur
Copy link
Contributor Author

@fsackur fsackur commented Jan 30, 2022

Thanks for the feedback, I have addressed the points. Can I get a re-review?

@anamnavi you asked me to pascal-case some URIs in some strings and comments. I've changed a few. Mainly, I've used Uri where the comment refers to the parameter and URI when expressing an actual URI. That's the style I've seen the most. It seems more natural in a sentence. Want me to change?

Copy link
Member

@alerickson alerickson left a comment

Looks good, just a couple of spots that both @anamnavi and I added suggested changes to be consistent with the Pascal casing of 'Uri'. Once those are in this should be good to go 😄

src/code/RegisterPSResourceRepository.cs Show resolved Hide resolved
src/code/Utils.cs Outdated Show resolved Hide resolved
src/code/SetPSResourceRepository.cs Outdated Show resolved Hide resolved
src/code/SetPSResourceRepository.cs Outdated Show resolved Hide resolved
src/code/SetPSResourceRepository.cs Outdated Show resolved Hide resolved
src/code/RepositorySettings.cs Outdated Show resolved Hide resolved
help/Register-PSResourceRepository.md Outdated Show resolved Hide resolved
help/Register-PSResourceRepository.md Outdated Show resolved Hide resolved
@fsackur
Copy link
Contributor Author

@fsackur fsackur commented Feb 1, 2022

I take that feedback to mean that URI should always be Uri in comments and strings, so I have changed all occurrences in 6c92c80

@alerickson
Copy link
Member

@alerickson alerickson commented Feb 2, 2022

Thanks for giving us a heads up on that @kilasuit! I'm going to open up an issue so that we can test and make sure we have the 'projectUrl' or 'projectUri' and 'iconUrl' or 'iconUri' in the appropriate places. I'll open up a new PR to fix any changes there and add you to review it (no pressure to if you'd rather not).

@fsackur
Copy link
Contributor Author

@fsackur fsackur commented Feb 3, 2022

I did publish a module to the gallery from this branch. I was then able to find it; it did have project and licese properties as set in my .psd1.

@fsackur
Copy link
Contributor Author

@fsackur fsackur commented Feb 3, 2022

@anamnavi thanks for all the work you and @alerickson have put into this review. I have taken your review comments to mean that I should change URI => Uri in ALL comments and strings, so I've done that in ALL occurrences (there were a few that you hadn't highlighted).

The only places where all-caps URI still appear:

Path                       LineNumber Line
----                       ---------- ----
.\CHANGELOG.md                     38 - Fix bug to register repositories with local file share paths, ensuring repositories with valid URIs can be registered.
.\test\PSGetModuleInfo.xml         34       <URI N="LicenseUri">https://github.com/PowerShell/SecretManagement/blob/master/LICENSE</URI>
.\test\PSGetModuleInfo.xml         35       <URI N="ProjectUri">https://github.com/powershell/secretmanagement</URI>
.\test\PSGetTestUtils.psm1        514 .LICENSEURI$(if ($LicenseUri) {" $LicenseUri"})
.\test\PSGetTestUtils.psm1        516 .PROJECTURI$(if ($ProjectUri) {" $ProjectUri"})
.\test\PSGetTestUtils.psm1        518 .ICONURI$(if ($IconUri) {" $IconUri"})

I can haz merge?

src/code/FindHelper.cs Show resolved Hide resolved
@@ -46,5 +46,5 @@ See change log (CHANGELOG.md) at https://github.com/PowerShell/PowerShellGet
}
}

HelpInfoURI = 'http://go.microsoft.com/fwlink/?linkid=855963'
HelpInfoUri = 'http://go.microsoft.com/fwlink/?linkid=855963'
Copy link
Member

@anamnavi anamnavi Feb 3, 2022

Choose a reason for hiding this comment

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

@fsackur can you please revert this change specifically? Like just change it back to "HelpInfoURI".
This variable is uppercased "URI" (i.e HelpInfoURI) in the module manifest files for packages on PSGallery. Example: https://www.powershellgallery.com/packages/PowerShellGet/2.2.5/Content/PowerShellGet.psd1 L286).

Copy link
Member

@alerickson alerickson Feb 3, 2022

Choose a reason for hiding this comment

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

Actually I just noticed 'ProjectUri' and 'LicenseUri' both have Uri in Pascal case, so I think we should leave this as is for now

Copy link
Member

@anamnavi anamnavi Feb 3, 2022

Choose a reason for hiding this comment

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

okay that makes sense. I'll merge the PR in then :)

@anamnavi
Copy link
Member

@anamnavi anamnavi commented Feb 3, 2022

@fsackur thank you for the hard work and being so diligent with these changes and also sharing the rule of thumb you used when changing URI => Uri in all strings and comments, that's helpful for us to know! Sorry for the late response and review, I was a little out of it after my booster shot. This looks good to me, only one minor change request, per my comment right above this one (it's just a capitalization change for consistency sake with other .psd1 files, and is just a small nitpick if you don't mind addressing that). Thank you for your contribution! :D

@anamnavi anamnavi merged commit 1269e4e into PowerShell:master Feb 3, 2022
10 checks passed
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

4 participants