organizeDeclarations by type

[oiuhr]

This one will be a longshot 🙃

This PR adds an ability to organize types by type rather than visibility (but its more like an attempt to make this rule a bit more extensible). Within our project somewhat similar fork lives for around a year atm.

Have no idea of what direction this PR should take, so any comments and suggestions are highly appreciated 🥹

[codecov]

Codecov Report

Attention: Patch coverage is 95.77922% with 78 lines in your changes are missing coverage. Please review.

Project coverage is 95.15%. Comparing base (de9d9aa) to head (642417a).

❗ Current head 642417a differs from pull request most recent head 90101e2

Please upload reports for the commit 90101e2 to get more accurate results.

Files	Patch %	Lines
Sources/Options.swift	84.75%	25 Missing ⚠️
Sources/FormattingHelpers.swift	95.52%	20 Missing ⚠️
Sources/GitHelpers.swift	82.85%	18 Missing ⚠️
Sources/ParsingHelpers.swift	94.82%	9 Missing ⚠️
Sources/Rules.swift	99.40%	3 Missing ⚠️
Sources/Inference.swift	93.93%	2 Missing ⚠️
Sources/ShellHelpers.swift	96.42%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1678      +/-   ##
===========================================
- Coverage    95.18%   95.15%   -0.04%     
===========================================
  Files           21       21              
  Lines        23040    23095      +55     
===========================================
+ Hits         21931    21975      +44     
- Misses        1109     1120      +11     

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

[calda]

This is pretty cool! People have been asking for this for quite a while so I'm excited to see you took it on :)

I left a brief review focused on cases that I think could affect the behavior of this rule for consumers of the existing --mode visibility behavior.

We'll want to include more test cases of the --mode visibility behavior with declarations that fall into the the new declaration type categories (swiftUIProperty, overriddenProperty, instanceLifecycle, etc) to make sure that we aren't unexpectedly changing the sort order. Here's an example where the new categories are unexpectedly leaking over into the --mode visibility behavior: #1678 (comment).

By the way, if you didn't know already, the existing sort order behavior comes Airbnb's Swift Style Guide. The rules are defined here:

• https://github.com/airbnb/swift#marks-within-types
• https://github.com/airbnb/swift#subsection-organization
• https://github.com/airbnb/swift#computed-properties-at-end

[calda]

I think this will be a breaking change for any existing users who use %c in their existing configuration. We should probably at least continue supporting %c.

Could we just use %c for both cases (visibility marks and type marks)? Is there a case where somebody would specifically want both of them in the same value? In the code it seems like only one will ever have a non-empty value at a time.

[oiuhr]

Just sticking with %c is failing when i want, for example, something like // MARK: - Functions (Public), but supporting %c is always an option 👍

[oiuhr]

Removed unnecessary templates, rolled back to %c — I guess it`s okay in a current state of the feature.

[calda]

We'll want to document the available modes somewhere. Perhaps Examples.swift is a good place to do this?

[oiuhr]

Done, thanks for the comment!

[calda]

What happens if the existing visibility organization mode encounters a SwiftUI property or SwiftUI function?

Here's an example of a case where this causes a behavior change:

func testSwiftUIVisibility() {
    let input = """
    class Test {

        func foo() -> Foo {
            Foo()
        }

        func bar() -> some View {
            EmptyView()
        }

        func baaz() -> Baaz {
            Baaz()
        }

    }
    """

    testFormatting(
        for: input, rule: FormatRules.organizeDeclarations,
        exclude: ["blankLinesAtStartOfScope", "blankLinesAtEndOfScope", "sortImports"]
    )
}

I'd expect this test case to pass, but it currently fails on this branch.

Before, all three of the declarations were part of the instanceMethod category, so the existing order was preserved. Now the bar() func is parsed into the swiftUIFunction category, so is unexpectedly sorted above the others.

I think we should probably maintain separate DeclarationType enums for each mode (e.g. VisibilityModeDeclarationType and TypeModeDeclarationType), to make it less likely for changes to one mode to affect another mode.

[oiuhr]

Reshuffled type(of: Declaration) a bit & wrote some tests for new cases (overriden/SUI properties/methods) in different modes to preserve the existing order.

I`ll go against your suggestion to stick with different enums here — current approach provides at least some flexibility to further improvements such as custom type ordering (via config/etc), while hardcoded order is... well... hardcoded 💀

Of course it`s up to discussion, but any way — your case with SUI properties is covered. I could not find how i can brake a new case for instance lifecycle thou

[calda]

Your approach looks good and I can see how it would be more flexible. Thanks!

[rakuyoMo]

An idea that may have nothing to do with this PR (but this PR gives me some hope):

organizeDeclarations will change the order of the code. It would be great to have an order attribute similar to the type_contents_order rule in SwiftLint, allowing people to customize the order.

[oiuhr]

organizeDeclarations will change the order of the code. It would be great to have an order attribute similar to the type_contents_order rule in SwiftLint, allowing people to customize the order.

I was thinking about custom parameterization, but i could not achieve anything pretty. For example:

• For visibility we can do something like

-- mode visibility
-- order <order for visibility, where 0 is beforeMarks and 7 is fileprivate> [0, 1, ...]

• Type goes the same, but with more args

-- mode visibility
-- order <order for type, where 0 is beforeMarks and 15 is conditionalCompilation>

• Things get weird where it comes for custom visibility & custom type. Currently we have 8 visibility categories and 16 type categories with 8 * 16 = 128 categories, each of which must be listed in the appropriate order 💀

Any thoughts?

[rakuyoMo]

I was thinking about custom parameterization, but i could not achieve anything pretty.

Things get weird where it comes for custom visibility & custom type. Currently we have 8 visibility categories and 16 type categories with 8 * 16 = 128 categories, each of which must be listed in the appropriate order 💀

Any thoughts?

You're right, there are indeed so many permutations possible.

First of all, I think different mode should have separate order options, such as --visibility-order and --type-order, so that there will be no confusion when setting.

Secondly, based on the latest version of SwiftFormat, if we have the following content:

class TestOrganizeDeclarations {
    public func fooTest() { }

    private let privateValue = ""

    public let publicValue = 1

    private var privateComputedValue: Int { 0 }
}

When I execute the command with --rules organizeDeclarations, the result is as follows:

+ // MARK: - TestOrganizeDeclarations
+
class TestOrganizeDeclarations {
+
+   // MARK: Public
+
+   public let publicValue = 1
+
    public func fooTest() { }

-   private let privateValue = ""
+   // MARK: Private

-   public let publicValue = 1
+   private let privateValue = ""

    private var privateComputedValue: Int { 0 }
}

The key is that publicValue is moved before the fooTest method.

This logic makes me speculate: there should be a certain order inside organizeDeclarations now, but it is not publicly exposed to allow customization, or it is not as comprehensive as type_contents_order.

I'm sorry that I didn't check the specific code implementation, I'm just speculating here.

Referring to this part of the logic (if it does exist), I wonder if it will be of any help to your work.

In the end, I think the 128 categories should look like a double loop: arrange the types under each access permission; or arrange the access permissions within each type. It may not be as scary in practice as it looks in numbers.

[oiuhr]

In the end, I think the 128 categories should look like a double loop: arrange the types under each access permission; or arrange the access permissions within each type. It may not be as scary in practice as it looks in numbers.

Yeah, thats an appropriate one :) Looking further to implement this one, but I guess in a different iteration

[calda]

I think it's fine to just say this is the default, rather than specifically necessary for backwards compatibility.

Suggested change

	**NOTE:** For backwards compatibility with previous versions, if no value is
	provided for `--organizationmode`, the default value for `--organizationmode` of `visibility` will be used.

[oiuhr]

✔️

[calda]

👌🏻

[calda]

Thanks!

[calda]

New changes are helpful, thanks!

I'd like to review this PR again, but currently the diff is busted because of all of the extra commits. Could you rebase off of develop so the PR has the correct set of commits?

I'd also like to run this updated rule implementation on the Airbnb app codebase to see if it applies any unexpected changes. I can do that next week after the holiday.

[oiuhr]

I'd like to review this PR again, but currently the diff is busted because of all of the extra commits. Could you rebase off of develop so the PR has the correct set of commits?

Yeah, sorry for the mess 🥹

[calda]

Hi @oiuhr, I ran this updated rule on Airbnb's app codebase and noticed one issue. I'm seeing declarations within #if blocks sorted incorrectly. Here are two new test cases that currently fail on develop, but pass if I revert this change:

func testOrganizeConditionalInitDeclaration() {
    let input = """
    class Foo {

        // MARK: Lifecycle

        init() {}

        #if DEBUG
        init() {
            print("Debug")
        }
        #endif

        // MARK: Internal

        func test() {}
    }
    """

    testFormatting(for: input, rule: FormatRules.organizeDeclarations, options: FormatOptions(ifdefIndent: .noIndent), exclude: ["blankLinesAtStartOfScope", "blankLinesAtEndOfScope"])
}

func testOrganizeConditionalPublicFunction() {
    let input = """
    class Foo {

        // MARK: Lifecycle

        init() {}

        // MARK: Public

        #if DEBUG
        public func publicTest() {}
        #endif

        // MARK: Internal

        func internalTest() {}
    }
    """

    testFormatting(for: input, rule: FormatRules.organizeDeclarations, options: FormatOptions(ifdefIndent: .noIndent), exclude: ["blankLinesAtStartOfScope", "blankLinesAtEndOfScope"])
}

I expect both of these examples to be left as-is. Would you have a chance to fix this issue?

No other issues as far as I can tell!

[oiuhr]

I ran this updated rule on Airbnb's app codebase and noticed one issue.

Thanks for your research! 🙏 I’ll try to investigate as soon as I can. One moment though — I see this PR as merged, should I open a new one or just reopen this? @nicklockwood

[calda]

Thanks! Would suggest just posting a follow-up PR with a fix for this issue.

[calda]

I went ahead and made the fix here: #1714