(alldup)(#2171) Update links and be explicit on binary name #2172
Conversation
Ensured links used https, pointed to the English site and removed the licenseUrl link as it is no longer available. Package did not uninstall at all and the install script wasn't clear.
|
✅ Package verification completed without issues. PR is now pending human review |
| fileType = $fileType | ||
| file = Get-Item $toolsPath\*.exe | ||
| fileType = 'EXE' | ||
| file = Join-Path -Path $toolsPath -ChildPath 'AllDupSetup.exe' |
There was a problem hiding this comment.
Don't use Join-Path, but rather follow the convention we typically use.
| file = Join-Path -Path $toolsPath -ChildPath 'AllDupSetup.exe' | |
| file = "$toolsPath\AllDupSetup.exe" |
Then to a search replace in the update.ps1 file similar to how we do it in the calibre package (in case the executable name changes in the future).
There was a problem hiding this comment.
Why? They both do the same thing, and Join-Path is safer.
There was a problem hiding this comment.
As you also indicated below, Join-Path is used in this repository.
There was a problem hiding this comment.
Then to a search replace in the update.ps1 file similar to how we do it in the calibre package (in case the executable name changes in the future).
👍 I'll look at that.
There was a problem hiding this comment.
Using Join-Path also makes it harder to do a search replace in the update.ps1 file (which we very commonly do).
It also goes against the consistency we have of not using it (unless when necessary).
| $installLocation = Get-AppInstallLocation "$packageName*" | ||
| if (!$installLocation) { Write-Warning "Can't find $packageName install location"; return } | ||
| Write-Host "$packageName installed to '$installLocation'" | ||
| Get-ChildItem -Path (Join-Path -Path $toolsPath -ChildPath '*.exe') | ForEach-Object { Remove-Item -Path $_ -ErrorAction SilentlyContinue } |
There was a problem hiding this comment.
The old code equivalent to this one is on purpose, it creates an ignore file if for some reason it fails to remove the executable (to prevent it from being shimmed).
It is also very rare that we make use of Join-Path in this repository.
There was a problem hiding this comment.
It is also very rare that we make use of Join-Path in this repository.
Rare, but not never?
There was a problem hiding this comment.
Almost never. Pretty much the only time we use Join-Path is if the situation requires it.
There was a problem hiding this comment.
Why are you adding a uninstaller script?
Chocolatey CLI is able to automatically uninstall inno setup installers without the uninstall script.
There was a problem hiding this comment.
This was so long ago I have no idea, but would likely have been it didn't uninstall for me. I'm not in the habit of adding uninstall scripts where they are not needed.
| @{ URL32 = $url; Version = $version } | ||
| @{ | ||
| URL32 = $url | ||
| Version = $matches.version |
There was a problem hiding this comment.
| Version = $matches.version | |
| Version = $Matches.version |
We have commonly used an uppercase M when using the Matches variables.
There was a problem hiding this comment.
That's very strange. $matches is just another variable and variables traditionally start with a lower case character. Why would you need to use an uppercase letter?
There was a problem hiding this comment.
We don't need it from a code standpoint. It is for consistency to keep the casing similar to what we use elsewhere.
There was a problem hiding this comment.
On this: $Matches is a variable being defined by PowerShell, so the approach I've generally seen uses that existing casing - similar to guidance around foreach vs ForEach-Object.
|
Dear contributor, As this PR seems to have been inactive for 30 days after changes or additional information |
|
✅ Package verification completed without issues. PR is now pending human review |
|
Dear contributor, As this PR seems to have been inactive for 30 days after changes or additional information |
|
Dear contributor, As this PR seems to have been inactive for 30 days after changes or additional information |
Description
Ensured links used https, pointed to the English site and removed the licenseUrl link as it is no longer available and refactored code to be more in keeping with the repository and to explicitly specify the embedded binary name.
Motivation and Context
Latest package failed Package Validator and the package did not meet the standards of what we have in this repository now.
How Has this Been Tested?
Tested in Chocolatey Test Environment.
Screenshot (if appropriate, usually isn't needed):
Types of changes
Checklist: