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

Add rule to insert a blank line following a switch case with a multi-line body #259

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

calda
Copy link
Member

@calda calda commented Feb 2, 2024

Summary

This PR proposes a new rule to insert a blank line following a switch case with a multi-line body:

Insert a blank line following a switch case with a multi-line body. Spacing within an individual switch statement should be consistent. If any case has a multi-line body then all cases should include a trailing blank line. The last switch case doesn't need a blank line, since it is already followed by a closing brace.

Autocorrect is implemented in the blankLineAfterMultilineSwitchCase and consistentSwitchStatementSpacing SwiftFormat rules, which were added in nicklockwood/SwiftFormat#1621.

Reasoning

Like with declarations in a file, inserting a blank line between scopes makes them easier to visually differentiate.

Switch statements with multi-line cases can become visually busy and cramped without blank lines between the cases. In this example, all of the cases start to bleed together, making it more difficult to read the code at a glance:

func handle(_ action: SpaceshipAction) {
  switch action {
  case .engageWarpDrive:
    navigationComputer.destination = targetedDestination
    warpDrive.spinUp()
    warpDrive.activate()
  case .enableArtificialGravity:
    artificialGravityEngine.enable(strength: .oneG)
  case .scanPlanet(let planet):
    scanner.target = planet
    scanner.scanAtmosphere()
    scanner.scanBiosphere()
    scanner.scanForArtificialLife()
  case .handleIncomingEnergyBlast:
    energyShields.engage()
  }
}

Blank lines between the individual cases make the switch statement much easier to read:

func handle(_ action: SpaceshipAction) {
  switch action {
  case .engageWarpDrive:
    navigationComputer.destination = targetedDestination
    warpDrive.spinUp()
    warpDrive.activate()

  case .enableArtificialGravity:
    artificialGravityEngine.enable(strength: .oneG)

  case .scanPlanet(let planet):
    scanner.target = planet
    scanner.scanAtmosphere()
    scanner.scanBiosphere()
    scanner.scanForArtificialLife()

  case .handleIncomingEnergyBlast:
    energyShields.engage()
  }
}

Blank lines are not required in cases where all of the switch cases are a single line, but are still permitted if it helps with readability:

// RIGHT. Since none of the cases are multi-line, blank lines are not required.
func handle(_ action: SpaceshipAction) {
  switch action {
  case .engageWarpDrive:
      warpDrive.engage()
  case .enableArtificialGravity:
      artificialGravityEngine.enable(strength: .oneG)
  case .scanPlanet(let planet):
      scanner.scan(planet)
  case .handleIncomingEnergyBlast:
      energyShields.engage()
  }
}

// ALSO RIGHT. Blank lines are still permitted after single-line switch cases if it helps with readability. 
func handle(_ action: SpaceshipAction) {
  switch action {
  case .engageWarpDrive:
      warpDrive.engage()

  case .enableArtificialGravity:
      artificialGravityEngine.enable(strength: .oneG)

  case .scanPlanet(let planet):
      scanner.scan(planet)

  case .handleIncomingEnergyBlast:
      energyShields.engage()
  }
}

Please react with 👍/👎 if you agree or disagree with this proposal.

@calda calda force-pushed the cal--switch-statement-spacing branch from 961ea92 to 369bee9 Compare February 2, 2024 00:25
@calda calda requested a review from bachand February 8, 2024 17:44
Copy link
Contributor

@bachand bachand left a comment

Choose a reason for hiding this comment

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

Nice work @calda . LGTM with some comments.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
}
}

// WRONG. While the `.enableArtificialGravity` case isn't multi-line, the other cases are.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

README.md Show resolved Hide resolved
}
}

// RIGHT. Since none of the cases are multi-line, blank lines are not required.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}
}

// ALSO RIGHT. Blank lines are still permitted after single-line switch cases if it helps with readability.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}
}

// WRONG. While it's fine to use blank lines to separate cases, spacing within a single switch statement should be consistent.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@calda calda force-pushed the cal--switch-statement-spacing branch from 097893d to 18c161c Compare February 13, 2024 20:31
@calda
Copy link
Member Author

calda commented Feb 13, 2024

There wasn't much public engagement on this PR, but three engineers expressed support for this rule internally on Slack.

@calda calda merged commit 4f369f6 into master Feb 13, 2024
5 checks passed
@calda calda deleted the cal--switch-statement-spacing branch February 13, 2024 20:34
Copy link
Contributor

@bachand bachand left a comment

Choose a reason for hiding this comment

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

Thanks @calda !

}
}
```
Complex switch statements are visually busy without blank lines between the cases, making it more difficult to read the code and harder to distinguish between individual cases at a glance. Blank lines between the individual cases make complex switch statements easier to read.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

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

Successfully merging this pull request may close these issues.

2 participants