`organizeDeclarations` by type

oiuhr commented 2 months ago

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[bot] commented 2 months ago

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).

:exclamation: 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 :warning:
Sources/FormattingHelpers.swift	95.52%	20 Missing :warning:
Sources/GitHelpers.swift	82.85%	18 Missing :warning:
Sources/ParsingHelpers.swift	94.82%	9 Missing :warning:
Sources/Rules.swift	99.40%	3 Missing :warning:
Sources/Inference.swift	93.93%	2 Missing :warning:
Sources/ShellHelpers.swift	96.42%	1 Missing :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rakuyoMo commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

calda commented 1 month ago

I went ahead and made the fix here: https://github.com/nicklockwood/SwiftFormat/pull/1714

nicklockwood / SwiftFormat

`organizeDeclarations` by type #1678

Codecov Report