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

feat: Determine our SwiftFormat rules - WPB-10848 #1928

Open
wants to merge 102 commits into
base: develop
Choose a base branch
from

Conversation

samwyndham
Copy link
Contributor

@samwyndham samwyndham commented Sep 13, 2024

Issue

As discussed in Linting & Formatting Improvements (@johnxnguyen please take a look at that document as you were away during the meeting) we plan to improve our linting and formatting of Swift code to cover the entire codebase with more rules.

This PR is a place of discussion of SwiftFormat rules only. A similar PR will be created in the future to discuss SwiftLint rules. Once we have consensus on both SwiftFormat and SwiftLint rules we can move forward.

This PR will never be merged to develop.

Nearly every rule is enabled in this PR including the ones the make me 🤮 - there are two exceptions:

  • acronyms as this is hard to use with coding keys.
  • docComments as this won't work with our floating style comments.

Personally I'm of the opinion that generally the more rules the better that lead to a standard style across the team, but if we have preferences which these rules go against perhaps we can assert them via linting instead.

Task

Each commit (approximately) in this PR introduces a new rule to the codebase. The rule name will be in the title of the commit and there may be some additional info about it in the commit description.

As a reviewer your job is to go through the PR commit by commit and note down any rules that you dislike or have concerns about.

⚠️ Please leave you review here: https://wearezeta.atlassian.net/wiki/spaces/IOS/pages/1454800899/SwiftFormat+review

I will leave this PR up for some time. Please enjoy bike shedding.

Checklist

  • Title contains a reference JIRA issue number like [WPB-XXX].
  • Description is filled and free of optional paragraphs.
  • Adds/updates automated tests.

@samwyndham samwyndham changed the title WIP feat: Determine our linting formatting rules - WPB-10848 feat: Determine our SwiftFormat rules - WPB-10848 Sep 27, 2024
@samwyndham samwyndham marked this pull request as ready for review September 27, 2024 15:23
@netbe
Copy link
Collaborator

netbe commented Sep 30, 2024

docComments as this won't work with our floating style comments.
what do you mean by that?

@samwyndham
Copy link
Contributor Author

samwyndham commented Oct 1, 2024

docComments as this won't work with our floating style comments.
what do you mean by that?

@netbe I mean that we often have a line of whitespace between the doc comment and the declaration:

/// Adds two numbers

func add(a: Int, b: Int) {
  // ...
}

Whereas the rule expects the following and behaves incorrectly without it.

/// Adds two numbers
func add(a: Int, b: Int) {
  // ...
}

Copy link
Contributor Author

@samwyndham samwyndham left a comment

Choose a reason for hiding this comment

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

Here is my review of the rules. I am generally happy with most rules. I have reservations about the following:

  • conditionalAssignment - I have mixed feelings about this one. I'm not super against it but feel that in some cases the content can become overly dense.
  • preferForLoop - Mixed feelings. I would prefer if it ignored one liner forEach statements which can be done with --onelineforeach option.
  • redundantType - This can be bad for compile times. If we do enable it we should also measure our compile times and add exceptions where necessary.
  • markTypes - Creates a lot of marks and I don't feel they are that useful for anything. If everything becomes a mark what is the point.
  • organizeDeclarations - Not against this, but I think this one requires a closer look and discussion. It moves our code around a lot.
  • wrapConditionalBodies - I don't like this one as I like single line early exits.
  • wrapSwitchCases - I don't like this one. It takes up unnecessary space in my opinion.

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

Successfully merging this pull request may close these issues.

2 participants