Editorial: Unify 'else' and 'otherwise'#3733
Conversation
|
@tc39/ecma262-editors For the single-line if/else where both branches return, do we want to just split them up into two steps? |
syg
left a comment
There was a problem hiding this comment.
After discussion with editor group, as an editorial convention, let's do:
- If a conditional has just two branches (i.e. not an if-else cascade like in some Math functions), and the two branches are symmetric and short, prefer a one-liner.
- If a conditional has more than two branches (i.e. is an if-else cascade), prefer multi-lines with early return without a preceding
Else,on branches after the first one. - If a conditional has just two branches but the complexity of the two branches are asymmetric (e.g. one branch has more steps than the other), prefer multi-line with early return without a preceding
Else,on the else branch.
syg
left a comment
There was a problem hiding this comment.
Good to land as is, can fixup afterwards.
|
Thank you all for the discussion. I've implemented the fixes for rules 2 and 3, which were mainly about short-circuiting (early returns) in If-steps and removing the 'Else'. There were 74 candidates (based on the grep below), 62 of which were fixable. These fixes cover the above suggestions. Currently, the first rule 'one-liner when symmetric and short' isn't implemented, which I plan to implement shortly after this. |
|
This is going to need a rebase. |
spec.html
Outdated
| @@ -29936,8 +29894,7 @@ <h1>isFinite ( _number_ )</h1> | |||
| <p>It performs the following steps when called:</p> | |||
| <emu-alg> | |||
| 1. Let _num_ be ? ToNumber(_number_). | |||
| 1. If _num_ is not finite, return *false*. | |||
| 1. Otherwise, return *true*. | |||
| 1. If _num_ is not finite, return *false*; else return *true*. | |||
There was a problem hiding this comment.
Let's invert this condition.
There was a problem hiding this comment.
Sure, I'll reflect the feedback on the following commit.
michaelficarra
left a comment
There was a problem hiding this comment.
LGTM otherwise! I'm looking forward to your follow-up PR.
eb22442 to
dbb9b0e
Compare
|
@jhnaldo Shouldn't the inline warnings only show up when the algorithm was previously understood by esmeta and becomes not understood due to new phrasing? What's going on here? Are these changes actually causing loss of precision? |
|
@michaelficarra You're absolutely right. I realize now that the current logic is too naive. It triggers warnings for any change in a line, even if the phrasing was already unsupported. This is likely causing the confusion you mentioned. I'll fix this to compare the base/head results so that only truly 'new' yet-phrases are reported. I'll update this as soon as possible. |
|
@jhnaldo There's no rush! Enjoy the holiday. |
|
I've implemented rule 1 (inline if-else) for simple returns of boolean and number. For each case, multilines are merged into single-line if-else. Also, minor short-circuit fixes for if/else-if were made. |
|
I attempted to work on cases other than simple returns, but it was difficult to proceed because the criteria for symmetric/asymmetric and short/long were unclear. For now, I have merged or split rather simple/straightforward if-else blocks based on a 180-character threshold. The remaining cases seem to require further discussion. I am attaching grep commands and histograms for reference. inline if-else histogram |
See new ECMA-262 convention tc39/ecma262#3733
See new ECMA-262 convention tc39/ecma262#3733
See new ECMA-262 convention tc39/ecma262#3733
See new ECMA-262 convention tc39/ecma262#3733
See new ECMA-262 convention tc39/ecma262#3733
See new ECMA-262 convention tc39/ecma262#3733
See new ECMA-262 convention tc39/ecma262#3733
See new ECMA-262 convention tc39/ecma262#3733
This PR implements points discussed in issue #3648
Although I've tried to replace every occurrence of 'otherwise' with 'else' in the algorithm steps, there were 16 cases where simply replacing with 'else' lead to awkward sentence. I listed them below.