AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 289 forks source link

Consider changing code style to allow var when the type is apparent #854

Closed Barsonax closed 4 years ago

Barsonax commented 4 years ago

Summary

We currently have a editorconfig with some style rules set. Consider changing these to make the code more readable, shorter etc.

NOTE: these changes do not change the semantics of the code. For newer C# features that might involve style changes we also have a different issue https://github.com/AdamsLair/duality/issues/742

Analysis

IDE0008: Use explicit type

csharp_style_var_for_built_in_types = false:error

Barsonax commented 4 years ago

done in #863

ilexp commented 4 years ago

I'm re-opening this because it seems that this change leads to the IDE actively promoting inconsistencies in some cases. There are two classes of issues I've seen in the codebase recently:

Inconsistent var / explicit combos

There are a lot of cases where the IDE will now suggest to use var in one line, but an explicit type in the next, which will turn local variable initialization blocks into an inconsistent mix of both. Either the entire block should use var, or the entire block should be explicit, but not both.

devenv_cy4mZfngEn

devenv_hw3QSLrmyw

devenv_YanN78JPPc

devenv_mR3voKfLsl

Suggesting to use var when the type is actually not apparent

In some cases, the IDE will suggest using var even though the actual type is implementation dependent and technically, a member lookup is required to determine it.

devenv_2yYcFT7BaQ

devenv_phSOxN69AA


To address this, I'd like to revert the linked editor config change and go back to preferring explicit types everywhere. I'm totally okay with manually deviating from this in places where it makes sense, but I don't think the IDE should continue to generally suggest this - seeing this effect, I think the change can easily cause more trouble than it's worth 😐

Barsonax commented 4 years ago

It shouldn't suggest var in these cases, there's some other setting doing this.

Barsonax commented 4 years ago

image Not sure what visual studio is doing here but this doesnt make any sense why it suggest var in one case but not in the other similar case? It shouldnt suggest var at all..

more weird stuff image

EDIT: @ilexp better just revert this. Its not doing what I suspected it to do but missed these edge cases during the tests... EDIT2: https://stackoverflow.com/questions/62943445/csharp-style-var-when-type-is-apparent-seems-to-be-inconsistent-sometimes maybe we get some answers

ilexp commented 4 years ago

EDIT: @ilexp better just revert this. Its not doing what I suspected it to do but missed these edge cases during the tests...

Done. Closing this for now until the topic comes up again, feel free to re-open as needed.