-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
Update to .NET Framework 4.6.2 and add No Games Found text #238
Conversation
This is also highly recommended, if we want to package the app with MSIX: |
Quick answer because it's been a while since you opened this pull request: .NET Framework update makes sense. IMO, it would be better if the "no games found" message was also displayed if you already have 10 known games, try to add an 11th one (for example because th20 just got released), try to select your whole Touhou games folder, and thcrap doesn't find anything new because your th20 copy that you totally acquired legally was in your Downloads folder instead - so, if the search doesn't find any new game. I was thinking about quickly implementing that myself, but I didn't do it yet. So, you can either try to argue about why your version is better, implement my idea, or wait until I take the time to implement it. |
Apparently the email notifications were in my spam and I haven't been checking Github that frequently. |
It's styled similar to an InfoBar showing an error. Only show when new games couldn't be found. Auto-hide after 3 seconds.
Check it now @brliron |
I have a comment for you, and many others for ReSharper. First, when it comes to making a commit with many changes to the code style of a file, I think it really helps to split into 2 commits: one for the code style changes, and one for the functional change. Now, on to ReSharper. I'll start by saying that I'm mostly a C and C++ programmer, not a C# programmer, and that some of the following thoughts may be biased towards that. I don't like C# trying to impose its own coding style. Because people will endlessly fight over which style is better - I've even worked at a place that used 3 spaces for indentation because there was a huge fight between people wanting 2 spaces and people wanting 4 spaces, and they settled this by taking the thing in the middle. And if Microsoft thinks they know which one is objectively better, they're wrong, they're just one more developer group with their own subjective view on that question.
if (_contextMenu != null)
{
return _contextMenu;
} I don't like the one-line syntax because seeing branches is vital to understand the control flow of a function, and a one-line if hides that if. This is even more important if that if has a return, because return is even more important than if to understand the control flow of a function. Like, when I read the changes to the visibility code around 180-189 (old file) / 191-194 (new file), I think ReSharper decided to unconditionally call GameJs.Title is a reference to the "title" field in the thxx.js files. As such, it should stay fully lowercase. game_id is also a reference to the I like being explicit on A variable starting with an underscore irks me as if I wasn't allowed to name my variable like that, because C defines identifiers in the global scope starting with an underscore as reserved. This is fine for this PR, but this is another example of Microsoft being wrong when they think they know what is objectively the best code style, I can't take that code style and bring it with me in C land. I don't like comments in the middle of the code just to tell a tool to shut up. I mean, this may be fine if said code will only be touched by one tool and never by anything else, but is ReSharper the only code styling tool that exist and will ever exist for C#? If not, why don't we include comments for these other tools? Will we end up with even more comments for the tools than we have code? Quick question about ReSharper adding I was almost about to say that its uses of the null coalescing operator was nice, until I noticed that it really likes to hide assignments in them. For example: Outside of that, there are some good suggestions in that ReSharper pass, like going from Oh, and the part of the commit that's actually related to the message looks good. I'll just need to test it to make sure it works properly, but it looks like it should. |
In the future, I will try and keep to this. Working in SAP has made me forgot all about managing commits.
They aren't just a development group. They're the developer of language. They provide the default. You can deviate, but you should have a good reason to do so. If you don't need to why use something else?
One-liner if-returns are popular as guards. Which allow you invert ifs to reduce nesting. I similary dislike the two-liners for the same reason you mention. I also personally like the three-liner. If you're okay with it I can set it as a solution-wide setting (
It could be possible to separate models for the json files in a completely separate folder, and set a custom or disable naming convention for it. The default for C# is to use
I don't know specially for comments, but seems ReSharper is compatible with
And what's wrong with different languages having different coding styles? I personally think it's having each own signature allowing me to easily tell them apart and not mix them is nice. But also once again, can be changed of course.
ReSharper also likes to merge declarations and assignments. https://www.jetbrains.com/help/resharper/2023.2/JoinDeclarationAndInitializer.html |
@brliron Kind reminder for PR. This discussion is also probably fit for elsewhere. |
Sorry, it took a lot longer than I expected, I got busy, I got inspired to work on other things, I got a bit ill, but let's get back to it. First, on using ReSharper at all. I'm not sold on its utility? From your 2 PRs, it mostly changed how the code looks. Sure, the code may look a bit better (which is subjective), but for the most part, it doesn't run better, and it isn't safer. Also, half of the things it found are things that Visual Studio already told me, and that I ignored because I'm an arrogant boy and I believe I know better than Microsoft (basically everything related to variable names and to As for the part about ReSharper being free for Open-Source developers, I don't like the idea of being tied to a licence that a company may or may not choose to give us. Like, I didn't make any commit in March, April and July 2023. If the licence renewal happened around any of these months, I wouldn't be eligible. It would also discriminate new developers if our setup process says "get ReSharper" but they can't do it for free. Anyway, back to the individual points.
I found 4 places where this was done: Page4.xaml.cs lines 46, 85, 114 / 135 / 141, and 447. 46: In my head, despite the additional indent level, the pre-ReSharper makes more sense. Before ReSharper, we have "If _contextMenu isn't initialized, initialize it. Then, in any case, return _contextMenu". Post-ReSharper, we have "If _contextMenu is initialized, return it. Otherwise, initialize it, and return the new _contextMenu". The 1st version makes slightly more explicit that we're returning the same _contextMenu in every case, with just an additional step when it's null. 85: Not needed, we know that newMenu is a MenuItem. If we want to fix it, we could replace 114 / 135 / 141: First, I already told you that separating functional commits and code style commits was better, you got my point, and I didn't plan to repeat it, but I spent 5 minutes trying to untangle the logic of what ReSharper did to this code before understanding that it also got a bit of functional change, with the addition of customDisplayName. 447: Not perfect, but ReSharper is pushing this bit of code towards a better direction. Combine both ifs into one and add a newline after the return, and we're good.
I didn't know this was configurable, this might be something I want (but for if (something) {
// a
}
else {
// b
} but I'm not sure it exists in this setting because it's not really common. And if we can't do that, I'm not sure whether I prefer the variant with
It might work for some things, but not for all of them. For example, as I said,
All our C# developers don't do enough C# to have these conventions ingrained into their brains, and for them, being more explicit and keeping the
I don't like ReSharper comments in the middle of a file, but I don't mind them in a separate file. |
Yeah, no worries. It seems my notifications also have gone to the void this time as well. I guess we can close this PR and I can re-do it without much of the style changes. Regarding .editorconfig, it is solution/project-wide, C# heavy example, for visual studio C++ usage Microsoft has this page,for C# settings Microsoft has this page, and for general info there's this page.
This is a bit worrying to me, though. I do not believe using C++ conventions in C# is the best of idea. Especially if it's hotchpotch of both. Sure, we can enable in some style-guide that we should be more explicit. Although, going against the naming conventions used, just because it's a common occurrence doesn't fit well with me. It's not as if you would see 'GameId' you wouldn't know what it is... |
@brliron If it's good with you then, I will close this PR an redo this without any formatting. We can then continue style talks in a separate issue. |
Sounds good. Btw, for the .NET version upgrade, we looked a bit into it, and 4.8 seems like a good target.
|
.NET Framework 4.6.1 is no longer available for download, while 4.6.2 is.
Fix #235