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

CasePathable.modify: Change XCTFail to runtimeWarn as per documentation #155

Closed djangovanderheijden closed 5 months ago

djangovanderheijden commented 5 months ago

As per the documentation:

If the enum’s case does not match the given case key path, the mutation will not be applied, and a runtime warning will be logged.

In reality, calling modify with a non-matching case will always call XCTFail. This PR adds ComposableArchitecture's RuntimeWarnings.swift file and uses runtimeWarn instead of XCTFail.

Alternatively, we could consider changing the documentation to clarify that the use of a non-matching case causes a runtime crash. I'm open to either solution, but a warning log seems a bit more graceful.

mbrandonw commented 5 months ago

Hi @djangovanderheijden, our XCTFail from xctest-dynamic-overlay does a runtime warning under the hood when not in tests.

So this does currently behave as described in the docs. Are you seeing otherwise? Also, this definitely should not crash the app. Are you seeing that?

djangovanderheijden commented 5 months ago

Hey @mbrandonw, interesting. I did see a crash in my application (with the debugger attached, but not while testing). I'll take another look and get back to you

djangovanderheijden commented 5 months ago

Alright, the issue seems unrelated to CasePaths (sorry about that, I should've read the error on my colleague's screen better) but I am definitely seeing crashes:

Screenshot 2024-04-05 at 20 08 24

This happens in a clean, newly created project that's ran for the first time. Here's a small repro:

@CasePathable
enum Foo {
    case bar(String)
    case baz
}

struct ContentView: View {
    var body: some View {
        Text("Foo")
            .onAppear {
                var foo = Foo.baz
                foo.modify(\.bar) {
                    $0 = "bar"
                }
            }
    }
}
mbrandonw commented 5 months ago

Ah I see, this will be fixed with https://github.com/pointfreeco/xctest-dynamic-overlay/pull/81. Thanks for the heads up!

djangovanderheijden commented 5 months ago

Nice! I'll close this.