Rename params: *url to *uri

[fsackur]

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 https://github.com/PowerShell/PowerShellGet/issues/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

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Documentation needed
    • Issue filed:
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and enumerated concerns in the summary.

[fsackur]

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.

[alerickson]

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!

[anamnavi]

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

[fsackur]

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]

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

[kilasuit]

@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]

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?

[alerickson]

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 😄

[fsackur]

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

[alerickson]

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]

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]

@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?

[anamnavi]

@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).

[alerickson]

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

[anamnavi]

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

[anamnavi]

@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