More .NET 8 code cleanup #116
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
veger
reviewed
May 8, 2024
shpaass
force-pushed
the
feature/more-code-cleanup
branch
from
6c61e3a
to
86294ca
Compare
|
Resolved merge conflict. |
Dorus
reviewed
May 9, 2024
Dorus
reviewed
May 9, 2024
shpaass
force-pushed
the
feature/more-code-cleanup
branch
from
6757002
to
c3f7b33
Compare
|
Resolved a merge conflict: The diff above shows that |
shpaass
added a commit
that referenced
this pull request
May 10, 2024
This inspection makes a field or method of a struct readonly if it's possible, because in general if you use a struct, you want it to be readonly. Otherwise you should've used a class.
The changed members did not access any instance data, so they could be marked as static.
Other than that, the solver names were not used anywhere.
Full name: CA1865-CA1867: Use 'string.Method(char)' instead of 'string.Method(string)' for string with single char Applied this inspection because the char version is faster.
I don't know why it didn't detect all applications of the inspections yesterday, so here we go again. This commit mostly converts empty array inits into []. It also does some conversions of logic expressions into "is, is not".
I don't understand how exactly this tangle of functions works, but the fix works and the bug is gone. Steps to reproduce a bug: 1. Select a sole recipe to produce electricity. You will be given a recipe through solar panels and accumulators. 2. Change accumulator twice. 3. Notice that after the first change, the accumulator doesn't change even though you selected a different one.
Merging this commit with the previous one about IDE0290 is pure hell, so I'm not doing that.
It's weird that there is an unused variable. tag: potential bug
In other words, write methods as lambdas if applicable. From one side, it shortens the method descriptions. From the other side, we need to set max line length around 150-180 for it to be readable. However, Visual Studio seems to not have a setting for that, so we'll need to curate line length ourselves. Looking at this lambda spaghetti, I start to suspect that Inversion of Control might be useful there, so we don't pass a GUI object through half the files.
csharp_style_expression_bodied_methods = true:suggestion Now that I read code a bit more, the arrow notation in methods doesn't look cursed anymore. The rule of thumb is that if you see "=>" after the method signature, that means that this method returns the value from what follows after the arrow.
Same as with methods, the more you work with that code, the less cursed it feels.
as with the previous changes to editorconfig a couple of commit before, the more you work with the code, the less you are weirded out by the arrow notation.
shpaass
force-pushed
the
feature/more-code-cleanup
branch
from
c3f7b33
to
d2434c0
Compare
|
Resolved more merge conflicts. The diff above shows that I dropped one of the arrow-conversions to not bother if I merged correctly or not. Also, the commit about |
Dorus
approved these changes
May 10, 2024
Dorus
reviewed
May 10, 2024
| @@ -215,7 +205,7 @@ private class ExportMaterial { | |||
| deflateStream.CopyTo(ms); | |||
| byte[] bytes = ms.GetBuffer(); | |||
| int index = 0; | |||
| if (DataUtils.ReadLine(bytes, ref index) != "YAFC" || DataUtils.ReadLine(bytes, ref index) != "ProjectPage") { | |||
There was a problem hiding this comment.
I missed this one, but this is not a valid refactoring.
This gives a new error in the latest master:
On a separate note: Export project page to cliborad exports json, import project page from clipboard expect base64. So i couldnt test this function, but as you see in the watch screenshot it's not working as intended.
shpaass
added a commit
that referenced
this pull request
May 10, 2024
The pattern matching changed the behavior of the code into the wrong one. See #116 (comment) Notice the refactor operator did warn this might not be a correct refactoring: 
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It turned out all those inspections that were re-enabled and didn't fix anything were just dormant for some reason. Perhaps I should've restarted Visual Studio for them to work.
This PR includes three parts:
The main reason for these changes is that we already enabled arrow notation for the properties, and the other parts of the code stop looking weird with arrows if you read them for a bit.
I put the potentially controversial changes at the end, so we can drop them easier if necessary.
QA: checked the basic functionality on an existing Pyanodons project file.