-
Notifications
You must be signed in to change notification settings - Fork 1k
Add projectUrl at the end of the description in the package selection #714
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
base: main
Are you sure you want to change the base?
Conversation
d9016e9
to
82253ec
Compare
@Ana06 , as per #708 (comment) this is the only way I figured out to make this work but the linter complains |
6056a3c
to
dde7a5f
Compare
dde7a5f
to
cda6eb1
Compare
Space between package description and URL cannot be extended, I modified the Y position as per the comment in the issue #708 (#708 (comment)) |
9c89d92
to
d6a1557
Compare
install.ps1
Outdated
} | ||
# Check if $projectUrl contains a valid URL | ||
if ($projectUrl -match "^http") { | ||
$packageObject = [PSCustomObject]@{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] why is this a PsCustomObject
instead of a normal hash table? 😕
$packageObject = [PSCustomObject]@{ | |
$packageObject = @{ |
install.ps1
Outdated
# Check if $projectUrl contains a valid URL | ||
if ($projectUrl -match "^http") { | ||
$packageObject = [PSCustomObject]@{ | ||
PackageName = $packageName | ||
PackageDescription = $description | ||
PackageUrl = $projectUrl} | ||
}else{ | ||
$packageObject = [PSCustomObject]@{ | ||
PackageName = $packageName | ||
PackageDescription = $description} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not duplicate code:
# Check if $projectUrl contains a valid URL | |
if ($projectUrl -match "^http") { | |
$packageObject = [PSCustomObject]@{ | |
PackageName = $packageName | |
PackageDescription = $description | |
PackageUrl = $projectUrl} | |
}else{ | |
$packageObject = [PSCustomObject]@{ | |
PackageName = $packageName | |
PackageDescription = $description} | |
} | |
$packageObject = @{ | |
PackageName = $packageName | |
PackageDescription = $description | |
} | |
# Check if $projectUrl contains a valid URL | |
if ($projectUrl -match "^http") { | |
$packageObject.PackageUrl = $projectUrl | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to create PSCustomObject
entries from the hashes for several reasons, including better readability and a more structured data format. Also the code is simpler and easier to read in the function Get-AllPackages
(here) where we need to get a flattened array of only entries with property PackageName
.
Additionally this is what Gemini responds to the question:
Hash vs. PSCustomObject
It's generally a better practice to use PSCustomObject entries instead of raw hashes for several reasons:Property Access: You can access properties with the dot notation (e.g., $entry.PackageName), which is cleaner and more readable than hash notation ($entry["PackageName"]).
IntelliSense: In many PowerShell editors, PSCustomObject provides IntelliSense for its properties, which makes writing and navigating code much easier.
Object-Oriented Structure: They behave more like objects, which is often a more natural fit for representing structured data like your package entries.
Cmdlet Compatibility: Many PowerShell cmdlets are designed to work seamlessly with objects, making it easier to pipe and process your data.
While hashes are flexible, converting them to PSCustomObjects is a recommended practice for better code structure and compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason Gemini is providing are IMO not relevant for this simple code and not even correct. For example you can use the dot notation for a simple hash table too:
> $hash = @{ name = "value" }
> $hash.name
value
While I think AI is a useful resource that can speed your code writing, I advise you to be careful with trust it blindly and always rely on official documentation and testing the code on your own.
I think it is overkilled to use an object for this use of case. But if you think the code in Get-AllPackages
is simpler, I have no strong opinion (note the suggestion of using a hash was a nitpick, only the code duplication was not, which is already solved.
install.ps1
Outdated
# Create a temporary, local variable with the correct URL for this specific link. | ||
$linkProjectUrl.add_Click({ | ||
$tempUrl = $this.Text | ||
[system.Diagnostics.Process]::start($tempUrl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Why are you using [system.Diagnostics.Process]::start
instead of the PS functionStart-Process
? This is already used in other links in the installer and I would change it there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no particular reason, I asked Gemini and I think it is better to use the built-int cmdlet Start-Process
generally but for this particular case it makes no difference and it seems slower. I changed it in the rest of the links as well
Feature |
[System.Diagnostics.Process]::Start() |
Start-Process |
Type |
.NET Framework Method |
PowerShell Cmdlet |
Speed |
Generally faster; direct method call. |
Slightly slower; involves command parsing. |
Parameters |
Limited to method overloads (e.g., arguments). |
Extensive, with options like -NoNewWindow, -Wait, etc. |
Error Handling |
Uses standard .NET exception handling. |
Integrates with PowerShell's error handling system ($ErrorActionPreference). |
Output |
Returns a System.Diagnostics.Process object. |
Returns a System.Diagnostics.Process object by default, but can be suppressed. |
Use Case |
Ideal for simple, high-performance tasks within a GUI where you just need to launch a process. |
Better for complex scripts where you need fine-grained control over the process, manage output, or use other PowerShell features. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sara-rn what do you mean that it seems slower? We should use the fastest option if this is noticeable for the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't experience any difference myself, this is what Gemini responded and maybe it's worth testing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think performance shouldn't be an issue for such a simple task (opening a link) and as long as we are using it consistently I have no strong opinion (note I added it as nitpick), but please test that there is no performance difference locally or double check this information in official documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree for this case, like I said I didn't notice any performance issue
Add ProjectUrl if it contains a valid URL to the package selection window
d6a1557
to
bb99139
Compare
Thanks @Ana06 for the review, I addressed some of the requested changes, see my comments above. |
} | ||
# Check if $projectUrl contains a valid URL | ||
if ($projectUrl -match "^http") { | ||
Add-Member -InputObject $packageObject -MemberType NoteProperty -Name "PackageUrl" -Value $projectURl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just the code I proposed (simpler and more readable)?
Add-Member -InputObject $packageObject -MemberType NoteProperty -Name "PackageUrl" -Value $projectURl | |
$packageObject.PackageUrl = $projectUrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because I am using the PsCustomObject
, see my comment #714 (comment)
That doesn't work when using a PsCustomObject
, see error : ForEach-Object : Exception setting "PackageUrl": "The property 'PackageUrl' cannot be found on this object. Verify that the property exists and can be set."
install.ps1
Outdated
# Create a temporary, local variable with the correct URL for this specific link. | ||
$linkProjectUrl.add_Click({ | ||
$tempUrl = $this.Text | ||
[system.Diagnostics.Process]::start($tempUrl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think performance shouldn't be an issue for such a simple task (opening a link) and as long as we are using it consistently I have no strong opinion (note I added it as nitpick), but please test that there is no performance difference locally or double check this information in official documentation.
Add
projectUrl
at the end of the package description if it contains a valid URL to the package selection windowcloses #708