disable no-lone-blocks rule in tests

[turadg]

Description

prompted by #4981 (comment)

We've decided to allow blocks even when they are redundant because it makes for more consistency with cases in which they're necessary. One could argue it's better to be able to tell from reading the code whether the block is necessary (i.e. that there's a new binding that should not escape the block). This allows it in tests only because in production code it is important that it's clear whether the block was necessary or not.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

[FUDCo]

My quick take on this one is I don't like it. The lone blocks in message-patterns.js are there to give stylistic consistency to the various subtests, some of which have local variables and some of which don't. Enforcing this lint rule messes up that consistency with a resulting loss in legibility. The rule is not solving any problem we actually have; it seems to serve no useful purpose. I say ditch it.

[Chris-Hibbert]

I prefer that we not use #region markers. They do nothing helpful in my IDE.

The rule is not solving any problem we actually have

I agree with this.

[turadg]

The lone blocks in message-patterns.js are there to give stylistic consistency to the various subtests, some of which have local variables and some of which don't. Enforcing this lint rule messes up that consistency with a resulting loss in legibility.

Ok, I've updated the PR to allow them in tests.

The rule is not solving any problem we actually have; it seems to serve no useful purpose. I say ditch it.

We don't really know that it's not preventing a problem. We may have no evidence of a problem because it's working.

I didn't disable it across the board because I think in production code (as opposed to tests) it's more important that it's clear whether the block was necessary or not.

[FUDCo]

I think in production code (as opposed to tests) it's more important that it's clear whether the block was necessary or not

I don't know why it's important to know if a block was necessary or not, which feels to me like asking if whitespace was necessary or not. Can you articulate an argument why anyone would ever care?

One important place where this can happen legitimately are switch cases. Are switch cases in regular code exempt from this rule?

[turadg]

I don't know why it's important to know if a block was necessary or not, which feels to me like asking if whitespace was necessary or not. Can you articulate an argument why anyone would ever care?

If I'm reading some contract code and I see (contrived),

let foo = 0;
{
  const tempValue = 1;
  foo += tempValue;
}
{
  const tempValue = 1;
  foo += tempValue;
}

I can understand why they used blocks. The rule allows that. Now if I see,

let foo = 0;
{
  foo += 1;
}
{
  foo += 1;
}

then as the reader I'll be scratching my head why blocks were used. I'll suspect it's a bug because they were completely unnecessary, as if wrapping a parenthetical in an extra pair. I will waste time investigating whether there's really a bug.

One important place where this can happen legitimately are switch cases. Are switch cases in regular code exempt from this rule?

I don't know exactly what you have in mind, but all the switch statements in the repo have no errors are warnings related to this.

[FUDCo]

With respect to switch statements, what I mean is that we have stuff like:

switch (foo) {
  case 1: {
    const zot = findMyZot();
    doStuff...
    break;
  }
  case 2: {
     doOtherStuff();
     more stuff...
     break;
  }
  case 3: {
    let wat = getFirstWat();
    even more stuff...
    break;
  }
  default: {
    Fail`this should never happen`;
  }
}

In this example, case 2 and the default case are "lone blocks" but getting rid of the blocks (and associated braces) for those two cases but not case 1 or case 3 would disrupt the rhythm of the code and make it harder to read. And if we later needed to introduce a name into the scope of, say, case 2, we'd have to remember to add the braces. This is the same reason we require loop bodies and if consequents to be blocks even if they are one line -- which as far as I know don't get flagged by the no-lone-blocks rule either.

[turadg]

With respect to switch statements, what I mean is that we have stuff like:

Ok, so stuff that's already in the repo. The rule isn't erroring on any of that. I've confirmed with the specific example above.

No changes requested?

[FUDCo]

Looks like we're good then.

[FUDCo]

I suppose relaxing the rule in tests is improvement. I'd relax it everywhere, but this is a step in that direction.