Feature/os detection#31
Conversation
…d of choosing the latest as default
|
I did some more work on this and feel like its in a good state right now, feel free to point out parts you are not satisfied with and I can go back and change them. The additional workflow now works as follows:
All the old options should still work the same, the new logic only triggers then the -e Flag is missing. |
aevitas
left a comment
aevitas
left a comment
There was a problem hiding this comment.
Absolutely fantastic work. I think this could mark the start of a version 2.0 of the patcher with how many features this introduces. I've left a few (sometimes nitpicky) notes here and there, but overall this looks very good.
| @@ -0,0 +1,106 @@ | |||
| namespace Patcher | |||
| { | |||
| using System; | |||
There was a problem hiding this comment.
The using directives should be above the namespace declaration for consistency
| /// <typeparam name="T">The type of a single option</typeparam> | ||
| /// <returns>The selected option from the list</returns> | ||
| public static T ShowSelectionMenu<T>(List<T> options, Func<T, string> optionLine) | ||
| { |
There was a problem hiding this comment.
Can't this just be a private method in the Program.cs file? I'm not big on Utility classes, much less when they only have a single utility.
There was a problem hiding this comment.
I was trying to keep the amount of Code in the Program.cs down to make it less cluttered and easier to read. If you prefer it in the Program.cs I can move it over, shouldn't be that bad.
| /// <param name="operatingSystem"></param> | ||
| public UnityInstallation(string installationLocation, OperatingSystem operatingSystem) | ||
| { | ||
| _installationLocation = installationLocation; |
There was a problem hiding this comment.
Since almost none of the API of this class would work when this is null, it'd probably make sense to add a precondition check along the lines of _installationLocation = installationLocation ?? throw new ArgumentNullException(nameof(installLocation));
| { | ||
| foreach (PatchInfo patch in patches) | ||
| { | ||
| if (Regex.IsMatch(this.Version, patch.Version)) |
There was a problem hiding this comment.
this isn't required here. Also, assuming this gets passed values such as 2020.1, does this work? I didn't realize you could use regular expressions like this.
There was a problem hiding this comment.
Overlooked the this when I was trying to remove all of them.
The Regex.IsMatch returns true if the pattern (patch.Version) is contained at least once in the input (Version), examples would be:
| patch.Version | Version | Result |
|---|---|---|
| 2018.4 | 2018.4.1f1 | true |
| 2018 | 2018.4.22f1 | true |
| 2020.2 | 2020.2.0a2 | true |
| 2019.3.6f1 | 2019.3.6f1 | true |
| 2019.3.6f1 | 2019.3.7f1 | false |
This allows us to specify patches for more than once specific Version of Unity, which is handy because I saw that for Windows the entire 2018 Release Cycle has the same PatchInfo.
One thing that might come back to bite us is that it will return the first patch.Version that matches, which might lead to problems when for Example 2020.2.1f1 needs a special Patch but the rest of the 2020.2 Cycle has the same Patch, then it might return the general 2020.2 one instead of the 2020.2.1f1 one. But I have not seen something like that thus far, so I figured we can work that out when it comes to it.
There was a problem hiding this comment.
It might also make sense to add some Unit Tests for that Method, if you tell me which Framework you prefer for Testing I can add some if you want.
| var installations = new UnityInstallation[directories.Length]; | ||
| for (var i = 0; i < directories.Length; i++) | ||
| { | ||
| installations[i] = new UnityInstallation(directories[i], operatingSystem); |
There was a problem hiding this comment.
Fairly sure you could just yield return these and forego allocating the array here.
There was a problem hiding this comment.
You are correct, although I also had to remove the try-catch since you can't yield return from within a try-catch block. But nothing here should throw an UnauthorizedAccessException so it should be fine
| _ => throw new ArgumentOutOfRangeException(nameof(operatingSystem)) | ||
| }; | ||
|
|
||
| if (Directory.Exists(path)) |
There was a problem hiding this comment.
Inverting the if here could reduce nesting:
if (!Directory.Exists(path))
return Enumerable.Empty<UnityInstallation>();
|
@aevitas Thanks for the Feedback, glad you like it. I think I resolved everything you mentioned, if I forgot something or you find something else let me know. |
|
Thanks for the great contribution! |
2 participants
This is just my initial WIP Draft for #28 some things definitely still need work and cleanup, but I feel like its a good Point to stop for now and have some discussions about my approach and if you had something different in mind or some input in general. Feel free to tear it apart :)