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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rule to always inline let keyword when destructuring enum cases and tuples #126

Merged
merged 4 commits into from
May 21, 2021

Conversation

calda
Copy link
Member

@calda calda commented May 12, 2021

Summary

This PR proposes a new rule to always inline the let keyword when destructuring enum cases and tupes

When destructuring an enum case or a tuple, place the let keyword inline, adjacent to each individual property assignment

// WRONG
switch result {
case let .success(value):
  // ...
case let .error(errorCode, errorReason):
  // ...
}

// WRONG
guard let case .success(value) else { 
  return 
}

// RIGHT
switch result {
case .success(let value):
  // ...
case .error(let errorCode, let errorReason):
  // ...
}

// RIGHT
guard case .success(let value) else {
  return
}

Reasoning

  1. Consistency: We should prefer to either always inline the let keyworkd or never inline the let keyword. In Airbnb's Swift codebase, we observed that inline let is used far more often in practice (especially when destructuring enum cases with a single associated value).

  2. Clarity: Inlining the let keyword makes it more clear which identifiers are part of the conditional check and which identifiers are binding new variables, since the let keyword is always adjacent to the variable identifier.

// `let` is adjacent to the variable identifier, so it is immediately obvious 
// at a glance that these identifiers represent new variable bindings
case .enumCaseWithSingleAssociatedValue(let string):
case .enumCaseWithMultipleAssociatedValues(let string, let int):

// The `let` keyword is quite far from the variable identifiers, 
// so its less obvious that they represent new variable bindings
case let .enumCaseWithSingleAssociatedValue(string):
case let .enumCaseWithMultipleAssociatedValues(string, int):

Please react with 馃憤/馃憥 if you agree or disagree with this proposal.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@bachand
Copy link
Contributor

bachand commented May 13, 2021

I am having trouble following 3) in the reasoning. Can you make it more clear to me?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@calda
Copy link
Member Author

calda commented May 14, 2021

I am having trouble following 3) in the reasoning. Can you make it more clear to me?

I removed this example because I don't think it added much value

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@calda calda merged commit 5ac3493 into airbnb:master May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants