Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial: Avoid preceding a multi-step Else with a collapsed single-step If #3043

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

gibson042
Copy link
Contributor

@gibson042 gibson042 commented Apr 14, 2023

We have a few cases of algorithms with steps like

1. …
1. If <condition>, <simple consequent>.
1. Else,
  1. <alternative step 1>.
  1. …
1. …

This makes it unnecessarily difficult to track expected behavior, especially when skimming, and I think the spec should have a convention (ultimately codified as a lint rule) to avoid mixing collapsed an non-collapsed steps at the same level of an If–Else construct.

The first commit of this PR replaces violations of that convention, and later commits (removable upon request) PRs #3044, #3045, and #3046 apply simplifications in the neighborhood of its changes.

@michaelficarra
Copy link
Member

I like all of these, but I would prefer they each be separate PRs. Please remove all but the first commit from this PR and I'll approve.

@gibson042 gibson042 force-pushed the 2023-04-if-else-block-agreement branch from 6693880 to 4a04fc6 Compare April 17, 2023 01:33
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. @bakkot Could we add an ecmarkup lint rule for this?

Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, I agree with this stylistic convention.

To preempt: I very much like single-line 1-armed ifs and would be opposed to changing those.

@ljharb ljharb added editor call to be discussed in the next editor call and removed editor call to be discussed in the next editor call labels Apr 19, 2023
@jmdyck
Copy link
Collaborator

jmdyck commented Apr 20, 2023

I believe this PR handles all the cases outlined in the description.

In addition to those, the spec has some similar cases that the editors might also like 'fixed'. (But I'm not saying that needs to be done in this PR.)

  • 10 cases where, between the collapsed single-step If and the multi-step Else, there's a single-step Else if.

  • 2 cases (NumberBitwiseOp, ByteListBitwiseOp) where single-step If/Else if is followed by:

    1. Else, _op_ is `|`. Let _foo_ be ...
    

    which would be a multi-step Else if we expressed it as

    1. Else,
      1. Assert: _op_ is `|`.
      1. Let _foo_ be ...
    

    (similar to what we do in BigIntBitwiseOp).

  • 23 cases where the imbalance is the other way around: a multi-step If followed by a single-step Else or Else if.

@michaelficarra
Copy link
Member

@jmdyck Yes, I would like all of these addressed.

@jmdyck
Copy link
Collaborator

jmdyck commented Apr 20, 2023

@jmdyck Yes, I would like all of these addressed.

In this PR or a separate one?

@bakkot bakkot mentioned this pull request Apr 20, 2023
@bakkot
Copy link
Contributor

bakkot commented Apr 20, 2023

Fixes for all of those are in #3048. The new lint rule caught all of them (as far as I can tell) other than the NumberBitwiseOp / ByteListBitwiseOp cases, which I addressed manually.

@michaelficarra
Copy link
Member

Closing in favour of #3048.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants