Progressively run files through SwiftFormat by either tweaking rules in `.swiftformat` or adding/removing files to it

[hboon]

The goal with SwiftFormat is to eventually have --enable indent uncommented in .swiftformat

We don't want a huge and unnecessary commit that modifies the entire codebase, so this GitHub issue is to do this:

1. Uncomment --enable indent in .swiftformat
2. pods/SwiftFormat/CommandLineTool/swiftformat --config .swiftformat <a small subset of codebase>
3. Undo (1), commit (2)
4. PR
5. Repeat 1-4 between other work, gradually

Feel free to suggest a better way.

One alternative that is cleaner:

1. Make a huge PR that adds these to every file:

//swiftformat:disable --indent
<code...>
//swiftformat:enable --indent

2. Uncomment --enable indent in .swiftformat
3. PR
4. Fix 1-2 files at a time
5. PR
6. Repeat 4-6 between other work, gradually or while touching files in other PRs

[hboon]

RFC @oa-s @eviltofu

[eviltofu]

Don't do it in one change. It will screw up a lot of the current work. Do bit by bit.

[hboon]

Yes, that's what I mean by "progressively run files"

[hboon]

@oa-s @eviltofu let's try this. Separate commits and not always necessary to do this for every PR. In fact, don't do it for every PR. Slowly. Eventually, we'll make --enable indent permanent.

[hboon]

Only keeping 1 assignee. @eviltofu would you help to track this and make sure it gets done by everyone every now and then?

[eviltofu]

@hboon what about adding // swiftformat:enable indent to each file we worked on daily?

[hboon]

Do you mean like if I am going to reformat X.swift and Y.swift now (doesn't actually matter if I am going to work on it, but probably good to), to add to the top and bottom of both files // swiftformat:enable indent and the corresponding comment to disable it; repeat and then eventually enable it in .swiftformat and removing these comments?

I don't know the exact comment to enable and disable indentation with it globally disabled in .swiftformat though. Do you?

But sounds workable, but we probably have to sweep at intervals to check if we have files that aren't formatted yet. But anyway, it would move us forward, so sounds good to me.

[eviltofu]

Ok it appears that at least one rule needs to be enabled in the configuration file. I've added // swiftformat:enable indent to the head of AccountsCoordinator.swift and it did not modify the file. If I add --enable indent in the .swiftformat file, AccountsCoordinator.swift is modified when I run SwiftFormat on the command line.

It looks like we might need to write a script that invokes SwiftFormat on individual files.

--enable indent modifies 374 out of 1439 files.

The project does not build after running SwiftFormat. There is one error.

[hboon]

Another way is to just enable the rule in .swiftformat and modify almost every file in the repo and adding the comment to disable it, and as we work through them, remove the comment for each file. There will be a big diff, but since they will be all for the first and last line, it might be ok. But the problem is, if we tweak the .swiftformat file we need to run through this again. Let me think about this a bit. Thoughts welcomed too!

One specific thing I still have against our (argularly mixed) format is wrapping of long lines. I know @oa-s prefers to manually break function calls and definitions by arguments. I prefer 1 long line especially since the editor can soft wrap, but I didn't mind breaking them up. But the problem I'm hitting with this hard-wrapped format ocassionally is it breaks git log -S <line changed>. An alternative is for @oa-s or whoever prefers it to have their local .swiftformat that formats it however they want and then for a pre-commit hook to make it a 1-liner again, but this, and possible changes means it complicates how we roll out .swiftformat changes.

So:

A) Maybe just roll it out like we talked about earlier (and still figuring out) in this issue and worry about further changes to .swiftformat later.

or:

B)

It looks like we might need to write a script that invokes SwiftFormat on individual files.

Seems like this is a simple and long-term approach. This is to change the build phase to run this right?

${PODS_ROOT}/pods/SwiftFormat/CommandLineTool/swiftformat --config .swiftformat modules/AlphaWalletHardwareWallet <more files, long list of files or directories>

Then we only need to modify this command when we add/remove files or "reset" everything when we change .swiftformat

[eviltofu]

I'd make it a shell script.

./Pods/SwiftFormat/CommandLineTool/swiftformat <file-name>
./Pods/SwiftFormat/CommandLineTool/swiftformat <file-name>

Once a file is under SwiftFormat, it will stay under its control.

[hboon]

Maybe more efficient like this:

./Pods/SwiftFormat/CommandLineTool/swiftformat \
   <file-name0> \
    <file-name1>

[hboon]

Once a file is under SwiftFormat, it will stay under its control.

Until .swiftformat is modified :)

[eviltofu]

Once a file is under SwiftFormat, it will stay under its control.

Until .swiftformat is modified :)

Won't it just reformat the files under it's control?

[hboon]

Yes it would, that's the problem. Say we change indentation from 4 to 2 and there are 200 files being modified.

[eviltofu]

Maybe we bite the bullet and change everything at once.

[hboon]

Just use the script like you described? This version: https://github.com/AlphaWallet/alpha-wallet-ios/issues/6583#issuecomment-1502629552

Then when we need to roll out again, just need to "reset" by modifying the script to include 0 files and progressively add them again.

This works right?

[eviltofu]

Just use the script like you described? This version: #6583 (comment)

Then when we need to roll out again, just need to "reset" by modifying the script to include 0 files and progressively add them again.

This works right?

If this is part of the build phase as a script, it will always format the file when we build.

[hboon]

If this is part of the build phase as a script, it will always format the file when we build.

I know. But we are talking about different things.

Let's say there are N Swift files in our repo, So this script would be when we want to enable SwiftLint for all of them (well we might just remove the script, but easier for this particular conversation):

./Pods/SwiftFormat/CommandLineTool/swiftformat \
   <file-name0> \
    <file-name1> \
    <file-name2>
    .. \
    <file-nameN>

1. We kick start immediately with no files:

./Pods/SwiftFormat/CommandLineTool/swiftformat \
   <no files>

(^ actually have to comment out the whole command but clearer this way)

2. We start including 2 files:

./Pods/SwiftFormat/CommandLineTool/swiftformat \
   <file-name0> \
    <file-name1>

3. Keep adding more and eventually we reach:

./Pods/SwiftFormat/CommandLineTool/swiftformat \
   <file-name0> \
    <file-name1> \
    <file-name2> \
    <file-name4> \
    <file-name5>

Maybe or may not be all the files yet.

4. We decide to modify .swiftformat again, this would suddenly modify 6 files. We don't want that, so we start with (1) again, and loop.

If we don't using this technique or include a comment to enable/disable SwiftLint, editing .swiftformat will always hit all N files. So I think we have do this. And we'll all do this. But as assignee for #6583, you'll bug us to continue to do it.

[eviltofu]

I propose we put all the tests into this first. Those are hardly touched.

[hboon]

Sure, go for it

[hboon]

Could also be a good way to figure out if there's anything else to format…

[eviltofu]

Initial SwiftFormat for Tests #6686

[eviltofu]

What is the next step? @hboon

[hboon]

You suggested this:

I'd suggest we next look at braces sortedImports

Shall we try them?

[hboon]

or would you like me to look at all the rules at one go and tweak the .swiftformat file, aiming to get the file done at one go?

[eviltofu]

I think we can do this in one go. You want all the commented commands to be enabled? Then we can see the results in the small sample set of files.

[hboon]

Ok, let's try that.

[eviltofu]

Should we try enabling this for the whole project? @hboon

[hboon]

@eviltofu try as in not going to commit — sure.

[hboon]

Would you try and then see if there's any problematic ones? With the last PR, I gave up scanning after looking through half of AlphaWalletFoundation.

[eviltofu]

Would you try and then see if there's any problematic ones? With the last PR, I gave up scanning after looking through half of AlphaWalletFoundation.

It compiles and tests do run (except one or two failures which also happen before the swiftformat run). I will scan through all the changed files and highlight non-formatting changes.

[eviltofu]

lazy private var selectedIndicator: UIView = { changed to private lazy var selectedIndicator: UIView = { in AccountViewCell.swift.

[eviltofu]

-    struct Keys {
+    enum Keys {
        static let developerExtrasEnabled = "developerExtrasEnabled"
        static let ClientName = "AlphaWallet"
    }

It changed a struct to enum BrowserViewModel.swift. It seems like if the struct has no data, it's converted into an enum. It also changed a class (UIKitFactory) into an enum. The class only had static functions.

[eviltofu]

open override func offsetForDrawing(atPoint point: CGPoint) -> CGPoint {

to

override open func offsetForDrawing(atPoint point: CGPoint) -> CGPoint {

in BalloonMarker.swift

[eviltofu]

scan(Optional<(Output?, Output)>.none) { ($0?.1, $1) } changed to scan((Output?, Output)?.none) { ($0?.1, $1) } in Control+Publishers.swift

[eviltofu]

}, completion: { _ -> Void in changed to }, completion: { _ in in TransitionButton.swift

[eviltofu]

UIView.spacer(height: ScreenChecker().isNarrowScreen ? 0: 7), changed the spacing around the : UIView.spacer(height: ScreenChecker().isNarrowScreen ? 0 : 7), in VerifySeedPhraseViewController.swift

[eviltofu]

case let .connect(redirectUrl, version, _): changed to case .connect(let redirectUrl, let version, _): in DeepLinkTests.swift

It's just a matter of using one style consistently throughout.

[eviltofu]

I have scanned through all the changes. PR is #6783.

[eviltofu]

I will turn on for all the tests. @hboon