Skip to content

Conversation

@magneticflux-
Copy link

@magneticflux- magneticflux- commented Jan 5, 2025

I spent a few hours learning how to use .NET and C# for this, so apologies if there are any issues!

I wanted to package qbittorrent-cli in Nixpkgs for NixOS and had issues because .NET 8 (the latest LTS) is the earliest version supported and I still had to mess around with stuff and add -p:TargetFramework=net7.0 to make some random internal library happy. When I went to update everything so I didn't have to use a workaround in the future, I realized many of the dependencies were quite old and/or deprecated so I just started updating everything, eventually spiraling into this.

These commits are ordered from least-to-most-invasive; if you don't want a bunch of formatting changes or refactors, I can undo some or all of them, or you can just use the first commit!

First commit

First, I did the following for each dependency: I did a binary search to find the latest version that compiled and ran without issue. Then, I went to the library's website and looked for the changelog or migration instructions to the next version. Most of these were renames or parameter/field additions, and I did what I thought made sense (using default/safe values, converting old values, etc.).

Changes required:

  • Updating McMaster.Extensions.CommandLineUtils revealed that it does not (and technically never did) support overriding inherited attributes ([Argument()], etc.). This meant I had to remove some "default" attributes that were "overridden" with more specific text.
  • Updating CsvHelper let me remove the UriConverter class since it now has one that does the exact same thing. It also renamed Configuration to CsvConfiguration, moved CultureInfo to a parameter, and replaced SanitizeForInjection with the more detailed InjectionOptions (which I converted the existing values to).
  • Portable.BouncyCastle was deprecated and replaced by BouncyCastle.Cryptography, which just removed a deprecated constructor for Pkcs12Store that I replaced with an equivalent Pkcs12StoreBuilder call.

Then, I updated .NET itself to the latest net9.0 and removed all #if blocks testing for NETFRAMEWORK (.NET Framework, the proprietary Windows framework released from 2002-2019), NET46 (the specific version of the .NET Framework released in 2015), NETCOREAPP2_0 (a specific version of the new open-source .NET released in 2017), and NET6_0_OR_GREATER (a version of the new open-source .NET released in 2021, or newer).

After removing all the never-taken #if branches, I removed the now useless EnumHelper.TryParse and replaced it with Enum.TryParse. I didn't remove EnumHelper completely since the IsDefined function using a reified generic parameter was still useful as a shortcut.

After that, I edited the project file to remove the net46 and mono targets and scripts, regenerated the DotSettings file to remove unused templates(?), removed the App.config workaround for old versions of .NET Framework, updated the .gitignore.

I chose .NET 9 since it is the newest, but .NET 8 is a LTS release and will be supported for 6 months longer (until Nov. 2026). The next LTS (.NET 10) is expected to be supported from Nov. 2025-2028. Nothing in this patch requires .NET 9, so I can rebase to .NET 8 if you prefer, but the support timelines are similar and it might make updating to .NET 10 easier? IDK, I'm just reading off of Wikipedia as I go 🤷.

I also believe that dropping support for Mono and .NET Framework and only using the main .NET is fine now that things seem to have settled down in the past few years. Reportedly (from some Reddit comments), a "self-contained" build on the new versions will still probably work on very old OSs like Windows 7 and Ubuntu 12.04, unless new features are used. But I don't think it matters too much since .NET already doesn't support OSs that are out-of-support themselves, and Mono is only really used for legacy software now as far as I know.

Second commit

I used JetBrains ReSharper to analyze the project's formatting and wrote all non-default values to a new .editorconfig.

Third commit

I ReSharper's "Reformat and Cleanup" refactoring to standardize the formatting and apply any automatic changes that do not affect behavior.

  • Rearranged some code blocks (moving inner classes before fields before methods and alphabetizing)
  • Used var for local variables when possible
  • Removed redundant casts, parentheses, semicolons, and imports

Fourth commit

I enabled language support for nullable type annotations and then ran ReSharper's and Roslyn's analysis on the whole project. I used the UI to group common errors and warnings and then manually reviewed all new errors and any warnings about nullable types. Finally, I manually reviewed warnings for "modernization"-type fixes (replacing a deprecated method, using the new switch expression, using new pattern-matching, etc.) and applied ones that "seemed good" (I know, a very helpful description from a C# beginner 🤣).


Finally, I'm not familiar with the packaging process under setup, but I think a lot of the setup code dealing with different runtimes (described here https://github.com/fedarovich/qbittorrent-cli/wiki/Setup-Advanced-PlatformAgnostic) can be simplified or removed. I hope it's not too much of a pain to update!

Now targets `net9.0` and the latest minor version of C# (the default for new projects), and gets rid of as much "net46 vs Mono" stuff now that .NET Core is just as good.
@magneticflux-
Copy link
Author

This doesn't work right now because I'm experimenting with the attribute inheritance problem. Hopefully I can upstream my fix here natemcmaster/CommandLineUtils#556, otherwise we'd have to stick with the older version or copy [Argument()] to all inheritors.

case string str when
Enum.TryParse(EnumType, str, !CaseSensitive, out _) || AllowEmpty && string.IsNullOrEmpty(str):
return ValidationResult.Success;
}
Copy link

Choose a reason for hiding this comment

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

What's the reason in all this rewriting?

There were nothing wrong with the original piece of code here. And it did excactly what it supposed to.
And you rewrite it to do the very same think. But in syntax you like. Why?
It just produces more changes without a solid reason. And that accordingly leads to increasing complicity of reviewing.

You named this commit "Update all dependencies and fix any issues / deprecations" and I don't see neither any depenecies to update or any issues / deprecations to fix in this pieces of code here and further (in this commit. I didin't check other ones).
And even so more it has nothing to do with a problem you described as the reason of your PR.

So what's the reason? If you really wanted to propose this code style to maintainer and contributors than you coluld release another separate PR with all this changes for team or maintainer to decide. But u said it's fixes when it actualy not.

ReadJsonFromResource(DarkResource)) ?? throw new InvalidOperationException());
_light = new Lazy<ColorScheme>(() => JsonConvert.DeserializeObject<ColorScheme>(
ReadJsonFromResource(LightResource)));
ReadJsonFromResource(LightResource)) ?? throw new InvalidOperationException());
Copy link

Choose a reason for hiding this comment

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

You at least should write down a message for this exceptions to make it easy for bug shooting. Especially so if you notice that author use todo-s like this one below
throw new Exception("The color scheme file is invalid."); // TODO: Throw specific exception

Here and further.

}

protected virtual IReadOnlyDictionary<string, Func<object, object>> CustomFormatters => null;
protected virtual IReadOnlyDictionary<string, Func<object?, object?>>? CustomFormatters => null;
Copy link

Choose a reason for hiding this comment

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

I became really hard to read

@MaxLevs
Copy link

MaxLevs commented Feb 15, 2025

You did great work but it looks like it will be painful to reveiw

@magneticflux-
Copy link
Author

I think there might be some confusion, this PR is composed of multiple commits, and the merging developer can choose to only take some of them. Like I said in the main body, all but the first commit are just blind refactoring anything with an automatic fix that JetBrains thinks is worse than a light warning, and then correcting the types of nullable variables based on how I think they are being used.

I don't expect the later commits to be used verbatim; at best, @fedarovich might be able to use them as a reference or starting point to migrate to using newer C# language features and APIs. The most important commit is the first one, the rest are just an exercise in eliminating warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants