Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Commit

Permalink
applicationName really is a required field, because UpdateManger will…
Browse files Browse the repository at this point in the history 
… throw if it is null
  • Loading branch information
@caesay
caesay committed Jan 1, 2022
1 parent efe4c75 commit 8f8530f
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/Squirrel/UpdateManager.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.Contracts;
Expand Down Expand Up @@ -34,7 +34,7 @@ public sealed partial class UpdateManager : IUpdateManager, IEnableLogger
IDisposable updateLock;

public UpdateManager(string urlOrPath,
string applicationName = null,
string applicationName,
string rootDirectory = null,
IFileDownloader urlDownloader = null)
{
Expand Down

2 comments on commit 8f8530f

@simader
Copy link

@simader simader commented on 8f8530f Jan 5, 2022

Choose a reason for hiding this comment

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

Hi caesay,
Why this parameter is now mandatory? There is still some code, which check if it is null:

this.applicationName = applicationName ?? UpdateManager.getApplicationName();

@caesay
Copy link
Member Author

@caesay caesay commented on 8f8530f Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

getApplicationName() throws if the applicationName parameter is null and your app is not installed. This is confusing for new users trying to adopt Squirrel. I have sat with two people and watched them try to implement it in an app and both of them got hung up on this issue, where they left this field as null and tried to run their app in Visual Studio only for it to crash immediately.

It is important you can create a new UpdateManager, even in a non-installed app - because checking the IsInstalledApp property or the CurrentlyInstalledVersion() method is the only way to know if you are installed or not, and leaving applicationName null causes the exception.

Therefore, I believe it should be a required field, unless you know you are already installed - and in that case, feel free to pass null explicitly.

Please sign in to comment.