YARC-Official / YARG

YARG (a.k.a. Yet Another Rhythm Game) is a free, open-source, plastic guitar game that is still in development. It supports guitar (five fret), drums (plastic or e-kit), vocals, pro-guitar, and more!
https://yarg.in/
GNU Lesser General Public License v3.0
746 stars 168 forks source link

EditorConfig: Enable "prefer braces" rule #865

Closed thomeval closed 2 months ago

thomeval commented 2 months ago

This PR adds one rule change to the project's .editorconfig file, which effectively disables the "prefer braces" Code Style rule. There doesn't seem to be a consistent style being applied in the YARG project, so this rule will explicitly permit either of these styles:

if (x == null) return;

OR

if (x == null)
{
    return;
}

I was able to find examples of both styles in existing YARG code. If the above rule is not present in the .editorconfig file, Visual Studio seems to default to whatever behaviour is configured locally, which can cause false warnings like these: image

EliteAsian123 commented 2 months ago

See, the thing is, personally, I think we should enforce braces and reformat all files to make sure that is followed.

We all agreed in the past that formatting something like this:

if (true) return;

...is harder to read compared to...

if (true)
{
    return;
}

...and...

if (true)
    return;

However, in my opinion, the example right above (new-line, no braces) is less maintainable. If you wanted to add to the if statement, you'd have to go about adding braces. Slight inconvenience, I know, but that's the reason why I prefer the other style. If you have the braces there already, you don't have to worry about adding them if you were to add more to the body of the if.

Gonna at the maintainers to get their opinion on this, because at the end of the day this is all subjective. @RileyTheFox @TheNathannator @sonicfind

Thanks!

TheNathannator commented 2 months ago

Definitely agreed on generally not allowing same-line statements for control flow. The only time I'll do this is if there's several single, related actions being performed in a row that all depend on different conditions, for example this method from PlasticBand-Unity:

public FiveFret GetFretMask()
{
    var mask = FiveFret.None;
    if (greenFret.isPressed) mask |= FiveFret.Green;
    if (redFret.isPressed) mask |= FiveFret.Red;
    if (yellowFret.isPressed) mask |= FiveFret.Yellow;
    if (blueFret.isPressed) mask |= FiveFret.Blue;
    if (orangeFret.isPressed) mask |= FiveFret.Orange;
    return mask;
}

I'm on the fence about disallowing the next-line no-braces style. I don't find it that big of a deal to have to add braces later on, and I personally don't like having redundant braces in most cases, especially for short-circuit return cases. if-else is a notable exception however, depending on the complexity of the single statement or the likelihood of further modifications. A single statement which ends up being split into multiple lines I'll wrap with braces for clarity, and if one path has braces added, I'll add them to the other to match.

Overall I'm very manual with this category of formatting choice lol, I don't consider any rule to be one-size-fits-all and I will happily break one if the situation calls for it.

RileyTheFox commented 2 months ago

Yeah, I prefer having braces. We decided that having braces with some rare exceptions is what we'll do.

thomeval commented 2 months ago

I tend to prefer having braces everywhere as well, so in that case I suggest we instead enable this rule, but at suggestion level instead of a warning. Warning level means " @EliteAsian123 will reject your PR if you don't fix this". If need be, we can suppress this rule in code blocks where it makes sense (like the one @TheNathannator pointed out) Any objections?