pointfreeco / swift-case-paths

🧰 Case paths extends the key path hierarchy to enum cases.
https://www.pointfree.co/collections/enums-and-structs/case-paths
MIT License
904 stars 105 forks source link

leadingTrivia indentation trimming calculation should not discard zero values #154

Closed djangovanderheijden closed 5 months ago

djangovanderheijden commented 5 months ago

With v1.3.1 documentation is now also included for the generated AllCasePaths struct. In order to achieve this the leadingTrivia for each case is appended to the generated properties. To ensure indentation matches, the code discards leading whitespaces from the trivia. When determining how many characters to trim, the code loops over each line in the leadingTrivia, discarding empty lines and counting the amount of prefixed whitespaces. The smallest value counted is then taken and used as the amount of whitespace to trim from each line.

There is an issue in this last bit of logic where lines without prefixed whitespaces are always ignored. This will cause these lines to be trimmed erroneously. Take the following example:

@CasePathable
enum MyEnum {
    // TODO: Implement this case
//    case one
    case two
}

Because the line // case one is ignored when calculating the minimum amount of whitespace, it is trimmed in the generated code:

public struct AllCasePaths {
    // TODO: Implement this case
      case one
    public var two: CasePaths.AnyCasePath<MyEnum, Void> {
        ...
    }

    …
}

This obviously causes a build failure.

The change removes the bit of logic where lines with zero whitespacing are ignored when calculating the minimum amount of whitespace. As far as I can tell this doesn’t have a negative impact. Tests still pass includingtestDocumentation.

djangovanderheijden commented 5 months ago

The underlying problem here is that Swift considers the first commented out case to be trivia relating to the second case even though it isn't. I don't think that's easy to fix though :)

stephencelis commented 5 months ago

@djangovanderheijden Thanks! Any chance you can add a macro test case for this? Would be good to capture the fix in test coverage.

djangovanderheijden commented 5 months ago

Hey @stephencelis, no problem! I added a small test that fails in the old situation and passes with the changes made.

I also noticed that the indentation of the line on which the property itself is declared (e.g. public var two: CasePaths.AnyCasePath<MyEnum, Void> {) was slightly different from the indentation found in other tests with the change. To combat this, I added a small tweak that removes trailing whitespaces excluding newlines from the very end of the leadingTrivia. All tests still pass and the change seems harmless enough so I figured I'd include it.