syntax-tm / SteamAchievementManager

Steam Achievement Manager
zlib License
149 stars 4 forks source link

Many enhancements #14

Closed UltraFeed closed 9 months ago

UltraFeed commented 9 months ago
syntax-tm commented 9 months ago

image

First off, I appreciate the attempted contribution. Please don't just run a formatter and then submit a 168 file 52 commit changed PR...

throw null exceptions correctly: from if (data == null) throw new ArgumentNullException("data"); to ArgumentNullException.ThrowIfNull(data);

Using the word "correctly" is a bold claim in the context of this PR and against my better judgement I'm going to be nice and just explain why you're wrong.

There is no "correct" way to do anything. If something isn't working or there's a bug then that's a different issue. There are multiple ways to solve a problem and the right one depends on many factors, one of which is personal preference. To assert that your personal preference is somehow more valid than someone else's is delusional. Personally, I use whichever makes more sense in the context and is more readable, looks better, etc.

ArgumentNullException.ThrowIfNull is usually not my preferred option. It looks like a normal static method invocation whereas checking for null and then throwing the exception is very distinct from the functionally relevant application code.

Add Dependabot support for automatic dependency updates. Add CodeQL

No.

Update Actions

This would be somewhat reasonable but much like the NuGet package updates if something is working and there's not a need to upgrade then it's not a high priority. I don't know if you tested them but the release one requires you to push a tag (it has workflow_dispatch but it's setup to run on a new or updated tag).

Add end_of_line = crlf and charset = utf-8-bom to editorconfig

No.

Update dependencies

It's not helpful to check every day to see if any NuGet package was updated and then immediately submit a PR. Unless you need some kind of functionality or it resolves an issue, then it's not urgent. I update packages fairly often when I am working on this (it comes in waves).