Disallow select expression-bodied members

[just-ero]

142 introduced changes which have since been agreed-upon to be unfavorable; namely these lines in the root .editorconfig:

https://github.com/leaderboardsgg/leaderboard-backend/blob/777ff4f40c66df16fe366a491972ce291d9b8ef3/.editorconfig#L102-L110

Expression-bodied members can break up the flow of reading and are otherwise detrimental to readability:

public class C
{
  private int _i;

  // Expression-bodied constructors are pretty exotic and not used or even seen a lot.
  // Yet, these are suggested as per the .editorconfig.
  public C(int i)
    => _i = i;

  // Ambiguous method; does this only increment _i, or also return the result?
  // Reader is forced to glance at return type to know.
  // Same goes for local functions.
  public void M()
    => _i++;
}

---

This issue's purpose is discussing and finding out which of these rules we truly want to enforce and which can be kept as-is. My opinion on this matter is that we should not simply remove any rules from the .editorconfig, as that will lead to inconsistent code style.

My proposal is the following:

csharp_style_expression_bodied_accessors       = when_on_single_line:suggestion
csharp_style_expression_bodied_constructors    = false:warning
csharp_style_expression_bodied_indexers        = when_on_single_line:suggestion
csharp_style_expression_bodied_lambdas         = when_on_single_line:suggestion
csharp_style_expression_bodied_local_functions = false:warning
csharp_style_expression_bodied_methods         = false:warning
csharp_style_expression_bodied_operators       = false:warning
csharp_style_expression_bodied_properties      = when_on_single_line:suggestion

Properties and indexers (basically just special properties) often serve a simple, single purpose that can easily be simplified to a single line, much like their accessors:

public int I => _i < 0 ? 0 : _i; // Single-line property.
public int this[string s] => _dict.TryGetValue(s, out int ret) ? ret : 0; // Single-line indexer.

private string _s;
public string S
{
  get => _s ?? ""; // Single-line accessor.
  set => _s = value; // Single-line accessor.
}

The above raises some issues again; expression-bodied properties and indexers cannot be set methods ('value' does not exist in this context). This is unlike methods, which indeed can be void when expression-bodied. This is once again inconsistent and subject to further scrutiny.

Expression-bodied lambdas when on a single line are obvious:

public List<int> M()
{
  return _strings
    .Select(s => int.Parse(s)) // Single-line lambda.
    .ToList();
}

---

With all this said, I'm happy to hear the maintainers' thoughts and am happy to implement the changes myself accordingly.

[TheTedder]

I don't wanna hear any more about the dang config until the gosh darn MVP is done, or at least until ONE feature is done! 😠😆

[just-ero]

@Dalet and @zysim have been talking about removing a selection of these rules from the file entirely. As mentioned in the original post, I believe this to be a bad idea, leading to inconsistent style (any contributor can just do whatever they feel like). This direction is harmful for the codebase in that it becomes difficult to browse and read the code in some places; a reader has to be constantly aware that the style can change at a whim and they have to adjust how they glance at member bodies. This adds unnecessary cognitive load on reading and contributing.

Beyond that, there are actually .editorconfig severities which can suppress the suggestions in an IDE, which seemed to be @Dalet's main issue. Those are silent (violations aren't visible to the user; still gets applied in formatting), and none (rule is suppressed completely; this should not be used). This means that removing the rules is not necessary.

Leaving the rules in the file still shows intent.

[just-ero]

I don't wanna hear any more about the dang config until the gosh darn MVP is done, or at least until ONE feature is done! 😠😆

Completely agree. I'm happy to have this issue lay dormant until a more appropriate time. I recommend adding a Post-MVP milestone and issue label here on GitHub that can be applied to issues such as this.

[TheTedder]

Your effort is better spent on making some features instead so do that please. We can worry about this later. It really doesn't matter one bit at this stage.

[just-ero]

Your effort is better spent on making some features instead so do that please. We can worry about this later. It really doesn't matter one bit at this stage.

I had already agreed with you, what gives?

[TheTedder]

What gives is that my phone is a bit far from the router and only your first message had shown up for me and I didn't know you were going to write another one.