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.

Sign up for GitHub

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

Jump to bottom

Add rule to prefer for loops over functional forEach { ... } method #234

Merged
merged 5 commits into from
Aug 8, 2023
Merged

Add rule to prefer for loops over functional forEach { ... } method #234

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
47b3bee
Add rule to prefer for loops over functional forEach { ... } method
calda Aug 1, 2023
bda51a6
Update README.md
calda Aug 2, 2023
8195dd0
Update README.md
calda Aug 5, 2023
fc19515
Clean up
calda Aug 7, 2023
13cb3a0
Merge branch 'master' into cal--forLoop
calda Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1542,6 +1542,8 @@ _You can enable the following settings in Xcode by running [this script](resourc
}
bachand marked this conversation as resolved.
Show resolved Hide resolved
```

</details>
Copy link
Contributor

Choose a reason for hiding this comment

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

NIce catch.


* <a id='anonymous-trailing-closures'></a>(<a href='#anonymous-trailing-closures'>link</a>) **Prefer trailing closure syntax for closure arguments with no parameter name.** [![SwiftFormat: trailingClosures](https://img.shields.io/badge/SwiftFormat-trailingClosures-7B0051.svg)](https://github.com/nicklockwood/SwiftFormat/blob/master/Rules.md#trailingClosures)

<details>
Expand All @@ -1562,6 +1564,42 @@ _You can enable the following settings in Xcode by running [this script](resourc
planets.first { $0.isGasGiant }
```

</details>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


* <a id='prefer-for-loop-over-forEach'></a>(<a href='#prefer-for-loop-over-forEach'>link</a>) **Prefer using for loops over the functional `forEach { ... }` method**, unless using `forEach` as the last element in a functional chain. [![SwiftFormat: forLoop](https://img.shields.io/badge/SwiftFormat-forLoop-7B0051.svg)](https://github.com/nicklockwood/SwiftFormat/blob/master/Rules.md#forLoop)
calda marked this conversation as resolved.
Show resolved Hide resolved
calda marked this conversation as resolved.
Show resolved Hide resolved
calda marked this conversation as resolved.
Show resolved Hide resolved

<details>

#### Why?
For loops are more idiomatic than the `forEach` method, and are typically familiar to all developers who have experience with C-family languages.

For loops are also more expressive than the `forEach` method. For loops support the `return`, `continue`, and `break` control flow keywords, while `forEach` only supports `return` (which has the same behavior as `continue` in a for loop).

```swift
// WRONG
planets.forEach { planet in
planet.terraform()
}

// WRONG
planets.forEach {
$0.terraform()
}

// RIGHT
for planet in planets {
planet.terraform()
}

// ALSO FINE, since forEach is useful when paired with other functional methods in a chain.
planets
.filter { !$0.isGasGiant }
.map { PlanetTerraformer(planet: $0) }
.forEach { $0.terraform() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Asking to learn (and I apologize for the lazy Q, as I know I could try this out...): at what point does the SwiftFormat rule implementation allow forEach(…)? Is it once the expression is multiple lines? I'd love if you want to link me to the code that makes this decision, if that's straightforward to do.

Copy link
Member Author

@calda calda Aug 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

By default the SwiftFormat rule avoids applying the rule in two cases:

  1. If the subject of the forEach call includes line breaks (handled here with an explicit check). For example, it would be weird to convert:
planets
  .filter({ !$0.isGasGiant })
  .map({ PlanetTerraformer($0) })
  .forEach({ $0.terraform() })

to a for loop formatted something funny-looking like

for planet in planets
  .filter({ !$0.isGasGiant })
  .map({ PlanetTerraformer($0) })
{
  print(planet)
}
  1. If the subject of the forEach call uses trailing closure syntax, like planets.filter { !$0.isGasGiant }.forEach { print($0) } (handled here by virtue of } not being handled in one of the switch cases -- should realistically be called out more explicitly).

We exclude this case because converting this to a for loop causes a new warning to be emitted:

// warning: trailing closure in this context is confusable with the body of the statement; pass as a parenthesized argument to silence this warning
for planet in planets.filter { !$0.isGasGiant } {
  print(planet)
}

A typical use case of chained functional operations hits both of these rules.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the subject of the forEach call uses trailing closure syntax, like planets.filter { !$0.isGasGiant }.forEach { print($0) } (handled here by virtue of } not being handled in one of the switch cases -- should realistically be called out more explicitly).

I improved how this is handled in the code here: nicklockwood/SwiftFormat@4d92ba2

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for explaining @calda and for that follow up!

```

</details>

### Operators

* <a id='infix-operator-spacing'></a>(<a href='#infix-operator-spacing'>link</a>) **Infix operators should have a single space on either side.** Prefer parenthesis to visually group statements with many operators rather than varying widths of whitespace. This rule does not apply to range operators (e.g. `1...3`) and postfix or prefix operators (e.g. `guest?` or `-1`). [![SwiftLint: operator_usage_whitespace](https://img.shields.io/badge/SwiftLint-operator__usage__whitespace-007A87.svg)](https://realm.github.io/SwiftLint/operator_usage_whitespace)
Expand Down
1 change: 1 addition & 0 deletions Sources/AirbnbSwiftFormatTool/airbnb.swiftformat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,4 @@
--rules trailingClosures
--rules elseOnSameLine
--rules sortTypealiases
--rules forLoop
calda marked this conversation as resolved.
Show resolved Hide resolved
Loading