`organizeDeclarations` custom naming & ordering #1736

oiuhr · 2024-06-18T10:18:08Z

This PR adds an ability to parametrize order inside organizeDeclarations categories & their respective marks.

I'm not sure about the design so it's up to discussion, but long story short:

We have 6 visibility categories (plus 2 for airbnb conventions, but i guess this number can increase without any major changes) and ~15 type groups. By providing parameters --visibilityorder we sort visibility categories (the same goes for the typeorder), and by --mode parameter we tell which category will be respected as primary sorting group. Each group either take a default mark comment value or custom parametrized with parameters --visibilitymark & --typemark, if provided.

codecov · 2024-06-18T10:30:23Z

Codecov Report

Attention: Patch coverage is 94.50549% with 20 lines in your changes missing coverage. Please review.

Project coverage is 95.21%. Comparing base (a9c61c1) to head (79518d3).

Files	Patch %	Lines
Sources/OptionDescriptor.swift	80.95%	16 Missing ⚠️
Sources/FormattingHelpers.swift	98.46%	4 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1736      +/-   ##
===========================================
- Coverage    95.49%   95.21%   -0.29%     
===========================================
  Files           20       20              
  Lines        23386    23580     +194     
===========================================
+ Hits         22332    22451     +119     
- Misses        1054     1129      +75

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

oiuhr · 2024-06-18T10:39:25Z

Current questions:

Is it okay if we cant filter out custom marks? For example: we were using default mark for instance lifecycle group, which was given an // MARK: Lifecycle mark. Then we change up to custom mark, smth like // MARK: Init and // MARK: Lifecycle is replaced. But if we jump from custom to default formatter simply have nothing to compare and just place // MARK: Lifecycle under previously used custom mark. Should we add something like --potentialCategoryMarks to filter them out if needed?
I couldn't find the way to somehow validate provided parameters. For example --visibilityorder clearly should have 6+ entries in it, all of which should be unique and valid 💀

calda · 2024-06-18T13:31:15Z

Sources/Examples.swift

+        [
+            beforeMarks,
+            instanceLifecycle,
+            open,
+            public,
+            package,
+            internal,
+            fileprivate,
+            private
+        ]


Since I assume the argument itself doesn't allow the [ and ] characters, maybe we should omit them from the default example?

Suggested change

[

beforeMarks,

instanceLifecycle,

open,

public,

package,

internal,

fileprivate,

private

]

beforeMarks,

instanceLifecycle,

open,

public,

package,

internal,

fileprivate,

private

calda · 2024-06-18T13:43:14Z

Is it okay if we cant filter out custom marks? For example: we were using default mark for instance lifecycle group, which was given an // MARK: Lifecycle mark. Then we change up to custom mark, smth like // MARK: Init and // MARK: Lifecycle is replaced. But if we jump from custom to default formatter simply have nothing to compare and just place // MARK: Lifecycle under previously used custom mark. Should we add something like --potentialCategoryMarks to filter them out if needed?

To me it seems fine to leave this as-is.

calda

Really cool, nice work! The implementation is really simple. Great to see how this builds on top of #1678.

I left some minor comments but otherwise this looks great to me!

The most important point is probably this discussion related to generating the DeclarationType for each declaration: #1736 (comment)

Sources/FormattingHelpers.swift

Sources/Examples.swift

Sources/FormattingHelpers.swift

Sources/OptionDescriptor.swift

Sources/Options.swift

Tests/RulesTests+Organization.swift

Sources/FormattingHelpers.swift

Tests/RulesTests+Organization.swift

calda

This is REALLY AWESOME! Nice work! I really like the way you implemented the validation. Great idea!

I left a few minor comments, but otherwise this is looking really good!!

calda · 2024-07-17T16:44:24Z

Sources/Examples.swift

@@ -1291,6 +1291,18 @@ private struct Examples {
    """

    let organizeDeclarations = """
+    Default value for `--visibilityorder`:


calda · 2024-07-17T16:45:18Z

Rules.md


 <details>
 <summary>Examples</summary>

+Default value for `--visibilityorder`:
+`["beforeMarks", "instanceLifecycle", "open", "public", "package", "internal", "fileprivate", "private"]`


Should we format these defaults in a way that would be accepted by the --visibilityorder argument?

So, instead, maybe:

Suggested change

`["beforeMarks", "instanceLifecycle", "open", "public", "package", "internal", "fileprivate", "private"]`

`beforeMarks, instanceLifecycle, open, public, package, internal, fileprivate, private`

yeah, thanks! this looks much better

calda · 2024-07-17T16:47:47Z

Sources/OptionDescriptor.swift

+            let essentials = Formatter.VisibilityType.essentialCases.map(\.rawValue)
+            for type in essentials {
+                guard order.contains(type) else {
+                    throw FormatError.options("--visibilityorder expects \(type) to be included")


Great idea!

calda · 2024-07-17T16:48:15Z

Sources/OptionDescriptor.swift

+                    guard let match = type.bestMatches(in: essentials).first else {
+                        throw FormatError.options(errorMessage)
+                    }
+                    throw FormatError.options(errorMessage + ". Did you mean '\(match)?'")


Great UX here!

calda · 2024-07-17T16:49:06Z

Tests/CommandLineTests.swift

@@ -262,7 +262,7 @@ class CommandLineTests: XCTestCase {
    func testHelpLineLength() {
        CLI.print = { message, _ in
            for line in message.components(separatedBy: "\n") {
-                XCTAssertLessThanOrEqual(line.count, 80, line)
+                XCTAssertLessThanOrEqual(line.count, 100, line)


We shouldn't change this

Suggested change

XCTAssertLessThanOrEqual(line.count, 100, line)

XCTAssertLessThanOrEqual(line.count, 80, line)

sry, rolled back

Sources/FormattingHelpers.swift

calda · 2024-07-17T16:54:16Z

Sources/FormattingHelpers.swift

            switch self {
            case .beforeMarks:
-                return "Before Marks"
+                "Before Marks"


You'll need to include the explicit return here since SwiftFormat supports old Swift versions without this newer feature

Suggested change

"Before Marks"

return "Before Marks"

calda · 2024-07-17T16:56:37Z

Sources/FormattingHelpers.swift

+        case .keyword("let"), .keyword("var"),
+             .keyword("operator"), .keyword("precedencegroup"):
+
+            if isOverriddenDeclaration && availableTypes.contains(.overriddenProperty) {


Nice! This looks great

calda · 2024-07-17T16:57:59Z

Sources/FormattingHelpers.swift

+        let customVisibilityMarks = options.customVisibilityMarks
+        let customTypeMarks = options.customTypeMarks
+
+        func parseMarks<T: RawRepresentable>(


Maybe this could put put somewhere else in the file rather than being nested in the categoryOrder method?

calda · 2024-07-17T16:58:17Z

Sources/FormattingHelpers.swift

+        let parsedVisibilityMarks: ParsedVisibilityMarks = parseMarks(for: customVisibilityMarks)
+        let parsedTypeMarks: ParsedTypeMarks = parseMarks(for: customTypeMarks)
+
+        func flatten<C1: Collection, C2: Collection>(


Same for this helper, maybe it doesn't need to be nested in this categoryOrder method

calda reviewed Jun 18, 2024

View reviewed changes

calda approved these changes Jun 18, 2024

View reviewed changes

calda reviewed Jun 18, 2024

View reviewed changes

Sources/FormattingHelpers.swift Show resolved Hide resolved

calda reviewed Jun 18, 2024

View reviewed changes

Tests/RulesTests+Organization.swift Show resolved Hide resolved

calda reviewed Jun 18, 2024

View reviewed changes

Tests/RulesTests+Organization.swift Show resolved Hide resolved

nicklockwood force-pushed the develop branch 3 times, most recently from c6722bc to c9f9e8b Compare June 23, 2024 07:50

nicklockwood force-pushed the develop branch 3 times, most recently from 610e36b to 15531fd Compare June 29, 2024 08:21

nicklockwood force-pushed the develop branch 2 times, most recently from 0bcca90 to a9c61c1 Compare July 10, 2024 19:07

Ryzhov Evgeniy Igorevich added 2 commits July 12, 2024 14:52

organizeDeclarations custom naming & ordering

cf102ec

PR comments changes

48c9120

oiuhr force-pushed the organizeDeclarations-custom-order branch from eb89782 to 48c9120 Compare July 17, 2024 02:17

Fix for custom category marks

79518d3

calda reviewed Jul 17, 2024

View reviewed changes

PR fixes

bee7158

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`organizeDeclarations` custom naming & ordering #1736

`organizeDeclarations` custom naming & ordering #1736

oiuhr commented Jun 18, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

oiuhr commented Jun 18, 2024 •

edited

Loading

calda Jun 18, 2024

calda commented Jun 18, 2024

calda left a comment •

edited

Loading

calda left a comment

calda Jul 17, 2024

calda Jul 17, 2024

oiuhr Jul 19, 2024

calda Jul 17, 2024

calda Jul 17, 2024

calda Jul 17, 2024

oiuhr Jul 19, 2024

calda Jul 17, 2024

calda Jul 17, 2024

calda Jul 17, 2024

calda Jul 17, 2024

	`["beforeMarks", "instanceLifecycle", "open", "public", "package", "internal", "fileprivate", "private"]`
	`beforeMarks, instanceLifecycle, open, public, package, internal, fileprivate, private`

	XCTAssertLessThanOrEqual(line.count, 100, line)
	XCTAssertLessThanOrEqual(line.count, 80, line)

organizeDeclarations custom naming & ordering #1736

Are you sure you want to change the base?

organizeDeclarations custom naming & ordering #1736

Conversation

oiuhr commented Jun 18, 2024 • edited Loading

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

oiuhr commented Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

calda commented Jun 18, 2024

calda left a comment • edited Loading

Choose a reason for hiding this comment

calda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`organizeDeclarations` custom naming & ordering #1736

`organizeDeclarations` custom naming & ordering #1736

oiuhr commented Jun 18, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

oiuhr commented Jun 18, 2024 •

edited

Loading

calda left a comment •

edited

Loading