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 wrapMultilineConditionalAssignment rule to wrap if / switch expressions to new line after assignment operator #1574

Merged

Conversation

calda
Copy link
Collaborator

@calda calda commented Nov 9, 2023

When discussing adopting the conditionalAssignment rule within Airbnb, we decided that the resulting code is easier to read if the if / switch expression is wrapped to a new line:

// Before: not wrapped, harder to read at a glance since the if statement is shaped strangely
let planetLocation = if let star = planet.star {
  "The \(star.name) system"
} else {
  "Rogue planet"
}

// After: wrapped after the assignment operator, the if statement has the same shape 
// we're used to seeing elsewhere (where the `if` starts on a new line).
let planetLocation =
  if let star = planet.star {
    "The \(star.name) system"
  } else {
    "Rogue planet"
  }

This is also the formatting that is used in most of the examples from the Swift Evolution proposal that introduced this feature (SE-0380), so seems likely to be a pretty common formatting preference.

This PR implements a wrapMultilineConditionalAssignment that wraps conditional assignment expressions in this way.

This PR also updates the indent rule to support this formatting.

Sources/Rules.swift Outdated Show resolved Hide resolved
@calda calda changed the title Add wrapMultilineConditionalAssignment rule to wrap if / switch expressions on to line after assignment operator Add wrapMultilineConditionalAssignment rule to wrap if / switch expressions to new line after assignment operator Nov 10, 2023
@nicklockwood nicklockwood force-pushed the develop branch 3 times, most recently from 1aa5771 to b04a159 Compare November 14, 2023 22:48
@calda calda force-pushed the cal--multilineConditionalAssignment branch 2 times, most recently from 94f6d52 to b0f4fcb Compare November 16, 2023 19:12
@calda calda force-pushed the cal--multilineConditionalAssignment branch from b0f4fcb to 0a56702 Compare November 16, 2023 19:13
@calda calda force-pushed the cal--multilineConditionalAssignment branch from ca6e3ed to e6b98ed Compare November 16, 2023 23:55
@calda
Copy link
Collaborator Author

calda commented Nov 16, 2023

@nicklockwood, all of the tests are passing now on this PR now that I found a solution for #1575

Copy link

codecov bot commented Nov 17, 2023

Codecov Report

Attention: 107 lines in your changes are missing coverage. Please review.

Comparison is base (e6928ea) 94.99% compared to head (727b791) 95.07%.
Report is 26 commits behind head on develop.

❗ Current head 727b791 differs from pull request most recent head 521698a. Consider uploading reports for the commit 521698a to get more accurate results

Files Patch % Lines
Sources/GrammaticalNumber.swift 41.97% 47 Missing ⚠️
Sources/Options.swift 85.35% 23 Missing ⚠️
Sources/GitHelpers.swift 82.85% 18 Missing ⚠️
Sources/Rules.swift 98.12% 9 Missing ⚠️
Sources/ParsingHelpers.swift 94.80% 4 Missing ⚠️
Sources/FormattingHelpers.swift 98.17% 3 Missing ⚠️
Sources/Inference.swift 93.93% 2 Missing ⚠️
Sources/ShellHelpers.swift 96.42% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1574      +/-   ##
===========================================
+ Coverage    94.99%   95.07%   +0.08%     
===========================================
  Files           20       20              
  Lines        21845    21969     +124     
===========================================
+ Hits         20751    20887     +136     
+ Misses        1094     1082      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@calda
Copy link
Collaborator Author

calda commented Nov 17, 2023

Attention: 1 lines in your changes are missing coverage.

Impressive! The code coverage bot highlighted a case I forgot to add a test for. Added a test for it in 727b791

@calda
Copy link
Collaborator Author

calda commented Nov 17, 2023

I ran both the conditionalAssignment and wrapMultilineConditionalAssignment rules on the Airbnb codebase and didn't notice any issues, so this is totally good to go / ready for review now 👍🏻

@nicklockwood nicklockwood merged commit 5d13c42 into nicklockwood:develop Nov 18, 2023
nicklockwood pushed a commit that referenced this pull request Nov 18, 2023
…ressions to new line after assignment operator (#1574)
nicklockwood pushed a commit that referenced this pull request Nov 18, 2023
…ressions to new line after assignment operator (#1574)
nicklockwood pushed a commit that referenced this pull request Nov 26, 2023
…ressions to new line after assignment operator (#1574)
nicklockwood pushed a commit that referenced this pull request Dec 2, 2023
…ressions to new line after assignment operator (#1574)
nicklockwood pushed a commit that referenced this pull request Dec 6, 2023
…ressions to new line after assignment operator (#1574)
nicklockwood pushed a commit that referenced this pull request Dec 23, 2023
…ressions to new line after assignment operator (#1574)
nicklockwood pushed a commit that referenced this pull request Dec 23, 2023
…ressions to new line after assignment operator (#1574)
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.

5 participants