Bug fixes and improvements for organizeDeclarations rule

[calda]

I ran the organizeDeclarations rule (#701) on our codebase last week and noticed a few issues. This PR includes several improvements and bug fixes:

Improvements

• It will update some malformed marks (e.g. with incorrect capitalization) to the correct format instead of adding duplicates.
• It won't insert category separators when there is only one category.

Bug fixes

• Fixed issue where lines with trailing comments would get broken up and cause compilation failures
• Fixed issue where situational-keyword identifiers (like NavigationBarType.static) would get misinterpreted as separate declarations and cause compilation failure
• Fixed issue where conditional compilation blocks (#if ... #endif) would not be parsed correctly
• Fixed issue where symbol imports (e.g. import class Module.Type) would not be parsed correctly

[coveralls]

Coverage increased (+0.03%) to 93.592% when pulling ad50c5d on calda:cal--organize-decls-fixes-and-improvements into fbb9e53 on nicklockwood:develop.

[nicklockwood]

@calda thanks for the improvements!

Something I'm struggling with at the moment is that the organizeDeclarations rule inserts blank lines at the start and end of scopes, even if the declarations are otherwise organized. This conflicts with the blankLinesAtStart/EndOfScope rules which remove these lines (and are enabled by default), resulting in a lot of spurious lint warnings.

Would it be possible to modify it not to do this? It doesn't seem like it's in scope of what the rule is trying to achieve. If this is part of the AirBnB style guide that you are trying to enforce, I think it would make more sense to make it a configuration option for the existing blankLinesAtStart/EndOfScope rules.

[calda]

Ahh I always forget that any mutation to the formatter will get flagged as a warning (even if it gets undone by a different rule). I’ll post a fix for the conflict with the blankLinesAtStart/EndOfScope rules

[calda]

@nicklockwood It seems like there are also conflicts between the existing enabled-by-default blankLinesAroundMark and blankLinesAtStartOfScope rules.

For example, this test below fails because blankLinesAtStartOfScope and blankLinesAroundMark take turns adding and then removing the blank line at the top of the scope:

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

        // MARK: Lifecycle

        init(json: JSONObject) throws {
            bar = try json.value(for: "bar")
            baz = try json.value(for: "baz")
        }

        // MARK: Internal

        let bar: String
        let baz: Int?
    }
    """

    testFormatting(for: input, rules: [
        FormatRules.blankLinesAroundMark,
        FormatRules.blankLinesAtStartOfScope,
        FormatRules.blankLinesAtEndOfScope,
    ])
}

It seems like there are valid cases where we should be able to insert blank lines at the start or end of the scope (like this example with behavior from the existing 0.45 release). I'm not sure what we can / should do about potentially-conflicting rules like these.

Thoughts?

[nicklockwood]

@calda generally I manage these by either modifying the rules not to conflict (e.g. I could change blankLinesAroundMark not to insert a blank line if it's the first line in a scope) or by using the orderAfter: parameter to ensure that one rule always wins (although that's unsatisfactory due to the linting noise).

I'm actually not sure what's best in this case. I'll give it some thought.

[calda]

or by using the orderAfter: parameter to ensure that one rule always wins (although that's unsatisfactory due to the linting noise)

If we update the linter to only compare the input and output source code (rather than simply listening for formatter mutations), then we could make this work by having blankLinesAtStartOfScope and blankLinesAtEndOfScope ordered after blankLinesAroundMark and organizeDeclarations.

Would there be challenges / performance issues / concerns with an approach like that?

[nicklockwood]

@calda I'm not sure how you would track which rule caused which change if you were just comparing the source at the start/end? But I'd be open to such a solution if you have a way to implement it.

Perhaps a hybrid approach would be possible, where changes are tracked at mutation time, but we retrospectively discard spurious changes by comparing the before/after source for each changed line? That should be possible since the formatted tokens retain the original line number.