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

Format the whole codebase with SwiftFormat #695

Merged
merged 11 commits into from
May 31, 2019
Merged

Format the whole codebase with SwiftFormat #695

merged 11 commits into from
May 31, 2019

Conversation

dusi
Copy link
Contributor

@dusi dusi commented May 30, 2019

📲 What

As title says

🤔 Why

Consistency consistency consistency + courage

🛠 How

Ran bin/format.sh . from project's root folder in order to format all the Swift files
(you can double check by running the same command that it won't change any files anymore)

🍍

  • You should first check .swiftformat configuration file and review the rules we're using over at https://github.com/nicklockwood/SwiftFormat/blob/master/Rules.md (there are some examples that will help you get started)
  • It's possible that due to the stripunusedargs rule, the function was stripped from argument names and would potentially fit on one line now (feel free to add suggestions if you spot this happening - there's no good way of automating this).
  • Please try to keep an eye on self reference which is now inserted by SwiftFormat in a closure context (so that we don't leak by not using [weak self])
  • Fun fact: The whole codebase was formatted in under 7s (that's how fast SwiftFormat is

✅ Acceptance criteria

  • There are no odd formatting choices done by the formatter
  • Playgrounds work 😄

Copy link
Contributor Author

@dusi dusi left a comment

Choose a reason for hiding this comment

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

@justinswart @ifbarrera @Scollaco @cdolm92 please feel free to review this PR that formats the whole codebase...more sets of eyes are welcome...please note that this is optional if you're not an assignee (who's name I'll randomly draw from my hat :)

Even though this is optional, I'd suggest you time cop yourself to 30 mins or an hour and scan through as much as you can so that you can familiarize yourself with the formatter changes - this will be the way going forward).

You can get familiar with our rules in .swiftformat config and check some examples over at https://github.com/nicklockwood/SwiftFormat/blob/master/Rules.md

It took me almost 4 hours to scan through the whole DIFF (yep, I'm never doing this again)...I was able to find some oddities and hopefully there's not much left.

Since Github won't be able to render everything and life is too short to read every single change, here are some tips for reviewers..

Suggestions

  • Scan through couple files under various areas of our codebase (Cells, ViewModels, KsApi/Models, etc., etc.)
  • Coordinate with other reviewers to not review the same thing (maybe one person can start from the top, one from the bottom, etc.)

@@ -1,26 +1,32 @@
# Indent by 2 spaces
# fileHeader
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to manually update SwiftFormat's config file to match the feature branch formatting

@@ -44,12 +46,14 @@ protocol NibLoading {

extension NibLoading {
static func fromNib(nib: Nib) -> Self? {
// swiftformat:disable indent
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An example of ignoring the formatting since the formatter didn't do a good job here (it's more readable for us this way)

@@ -66,6 +62,6 @@ extension LensHolder where Object: WebViewControllerProtocol {

extension Lens where Whole: WebViewControllerProtocol, Part == WKWebView {
internal var scrollView: Lens<Whole, UIScrollView> {
return Whole.lens.webView..Part.lens.scrollView
return Whole.lens.webView .. Part.lens.scrollView
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a range (--ranges no-space) but an operator so should be fine

@@ -1709,7 +1759,7 @@ internal final class RewardPledgeViewModelTests: TestCase {

func testOrLabelHidden() {
//todo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clearly todos are an exception (we can fix this manually or not)

@nonobjc public static func hexa(_ value: UInt32) -> UIColor {
let a = CGFloat((value & 0xFF000000) >> 24) / 255.0
@nonobjc static func hexa(_ value: UInt32) -> UIColor {
let a = CGFloat((value & 0xFF00_0000) >> 24) / 255.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks odd

Copy link
Contributor

Choose a reason for hiding this comment

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

How about we added // swiftformat:disable numberFormatting for this file?

Copy link
Contributor Author

@dusi dusi May 31, 2019

Choose a reason for hiding this comment

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

I've looked at the rules docs and it looks like hex grouping threshold was (4,8) by default so I've actually changed this to "ignore" for now

@ksr-ci-bot
Copy link

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@dusi
Copy link
Contributor Author

dusi commented May 31, 2019

An example of code change that makes things more readable..

Screen Shot 2019-05-30 at 4 13 03 PM

@dusi
Copy link
Contributor Author

dusi commented May 31, 2019

@justinswart
Copy link
Contributor

Please try to keep an eye on self reference which is now inserted by SwiftFormat in a closure context (so that we don't leak by not using [weak self])

Swift didn't allow references to self inside a closure without explicitly calling self before so I don't think this would have happened? Also, regardless of whether self is referenced, it would still be synthesized by the compiler I believe.

@@ -35,12 +35,14 @@ public enum Storyboard: String {
case Video
case WebModal

public func instantiate<VC: UIViewController>(_ viewController: VC.Type,
inBundle bundle: Bundle = .framework) -> VC {
public func instantiate<VC: UIViewController>(
Copy link
Contributor

Choose a reason for hiding this comment

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

This can fit on one line now 👍

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

This looks great. I skimmed around the codebase, everything looks good and just had the one suggestion to disable numberFormatting in that file. 🚢

@dusi dusi changed the title Format the whole codebase with SwiftLint Format the whole codebase with SwiftFormat May 31, 2019
@dusi dusi merged commit 520d0a3 into master May 31, 2019
@dusi dusi deleted the swift-format branch May 31, 2019 19:29
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.

3 participants