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

Enable SwiftFormat wrap rule with an enforced max width of 130 characters #132

Merged
merged 3 commits into from
Sep 9, 2021

Conversation

calda
Copy link
Member

@calda calda commented Aug 25, 2021

Summary

This PR enables the SwiftFormat wrap rule in airbnb.swiftformat. We would begin enforcing a maximum line width of 130 characters, and automatically wrap code to sit within the max line width. We would continue recommending (but not enforcing) a maximum line width of 100 characters.

Reasoning

We should begin enforcing our maximum column width rule with a linter ("We strive to make every rule lintable").

SwiftFormat's wrap rule is not perfect, but it follows our style guide / existing informal style preferences a large majority of the time (in general, it is more successful at wrapping longer lines). 100 characters seems too strict to enforce with the SwiftFormat wrap rule at this time, given its current behavior. Enforcing a "more generous" maximum width of 130 characters results in a fewer cases of "weird" wrapping, and is better than the status quo (where we make a recommendation, but enforce nothing).

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

resources/xcode_settings.bash Outdated Show resolved Hide resolved
@erichoracek
Copy link
Collaborator

erichoracek commented Aug 25, 2021

Since we have never enforced a maximum column width of 100 characters, it has never really been followed in practice. In my opinion, 100 chars is too strict to enforce automatically with a linter. Within our codebase, it appears that folks have generally followed a maximum character width of ~120-130. Adopting a "more generous" maximum width of 130 characters also results in fewer cases of "weird" auto-wrapping.

Personally I find that 120-130 characters is simply too long to comfortably fit two editor panes side-by-side on a laptop display, and furthermore I find that lines over 100 characters in length (e.g. doc comment blocks) are much harder to read, which is generally in line with typography recommendations:

Shorter lines are more comfortable to read than longer lines. As line length increases, your eye has to travel farther from the end of one line to the beginning of the next, making it harder to track your progress vertically. Aim for an average line length of 45–90 characters, including spaces.


Adopting a "more generous" maximum width of 130 characters also results in fewer cases of "weird" auto-wrapping.

I posit that once we start linting for line length folks will see the formatting results and clean up cases where the linter wrapped code in a "weird" way that made things confusing. It might be helpful to discuss a few real-world examples where the 100 character max resulted in weird or less readable results so we can see whether there's ways to make them better without adjusting the max line length.

@calda
Copy link
Member Author

calda commented Aug 25, 2021

I think a max width of 100 characters is a good recommendation, but I'm not sure it's suitable as a strictly-enforced maximum.

When evaluating style guide changes, I think it's important to consider how folks are currently writing code in practice. While we have historically recommended a max width of 100 characters, this hasn't been followed very broadly.

One reasonable proxy metric for current practices is the total diff count of a commit that reformats our codebase to follow a given max character width:

Max Width Diff Count Files Affected
100 +165,000 / -64,000 10,000 files
120 +66,500 / -26,000 6,000 files
130 +45,000 / -18,000 4,600 files
160 +17,500 / -8,500 2,400 files

Enforcing a max width of 100 characters results in a significantly more changes to our codebase, which suggests to me that it is too strict. I think 130 is a reasonable choice, out of these options, since it reformats lines that are "obviously too long" without being too invasive to existing practices and preferences.

@erichoracek
Copy link
Collaborator

erichoracek commented Aug 25, 2021

To clarify, I would be fine with keeping the editor page guide at 100 as the preferred line length, and then having a the formatter enforce something a bit longer (e.g. 130), as the formatting pass by definition won't be extending any lines that fall under the limit.

If we conflate the enforcement threshold with preferred threshold I think we get the worst of both worlds—code that is too long to fit in split editors on small screens and wrapped prose (e.g. doc comment blocks) that is not very readable due to its line length.

@bachand
Copy link
Contributor

bachand commented Aug 25, 2021

To clarify, I would be fine with keeping the editor page guide at 100 as the preferred line length, and then having a the formatter enforce something a bit longer (e.g. 130), as the formatting pass by definition won't be extending any lines that fall under the limit.

I think this is a good balance. I continue to believe that 100 characters is preferable. Code is easier to read/edit/maintain/understand when you can look at it without having to turn your head side to side to read long lines.

@bachand
Copy link
Contributor

bachand commented Aug 25, 2021

I posit that once we start linting for line length folks will see the formatting results and clean up cases where the linter wrapped code in a "weird" way that made things confusing. It might be helpful to discuss a few real-world examples where the 100 character max resulted in weird or less readable results so we can see whether there's ways to make them better without adjusting the max line length.

I also agree that it would be helpful to understand where this would break down with some examples.

@bachand
Copy link
Contributor

bachand commented Aug 25, 2021

I am supportive of enabling some automatic wrapping though I am not ready to 👍 an increase to the preferred max line length with the data that I've seen.

@calda
Copy link
Member Author

calda commented Aug 25, 2021

I think I can support recommending a maximum line width of 100 characters, and enforcing a maximum line width of 130 characters. I think enforcing a maximum is strictly better than the status quo (where we make a recommendation but enforce nothing).

@bachand
Copy link
Contributor

bachand commented Aug 25, 2021

I think enforcing a maximum is strictly better than the status quo (where we make a recommendation but enforce nothing).

💯

@calda
Copy link
Member Author

calda commented Aug 26, 2021

I updated this PR to continue recommending a max line width of 100 characters (while enforcing a max line width of 130 characters).

@calda calda changed the title Enable SwiftFormat wrap rule with a max width of 130 characters Enable SwiftFormat wrap rule with an enforced max width of 130 characters Aug 26, 2021
README.md Outdated Show resolved Hide resolved
Co-authored-by: Eric Horacek <horacek.eric@gmail.com>
@calda calda merged commit d6d5c15 into airbnb:master Sep 9, 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
Development

Successfully merging this pull request may close these issues.

4 participants