EmbeddedChannel: use tryAs() not forceAs()

[ianpartridge]

This is really a question - I am wondering why EmbeddedChannel calls forceAs() instead of tryAs() when attempting to read from a [NIOAny]. As the primary usecase of EmbeddedChannel is in testing why not return nil from readInbound() (or throw) so the user can handle that in their test?

[swift-nio-bot]

Can one of the admins verify this patch?

[swift-nio-bot]

Can one of the admins verify this patch?

[weissi]

@ianpartridge that's a good question, I don't have a strong opinion. Downside is obviously that we can then no longer distinguish between empty and wrong type. The other question is if this is SemVer major... @normanmaurer / @Lukasa ?

[normanmaurer]

hmm... I think if we do this we should throw as otherwise it is impossible to tell if it was empty or not as @weissi stated. And yes I think thats a semver major

[ianpartridge]

I agree that throwing would be best, and this is SemVer major unless we did something like:

public enum EmbeddedChannelReadInboundError: Error {
    case wrongType
}

extension EmbeddedChannel {
    public func tryReadInbound<T>() throws -> T? {
        return try readFromBuffer(buffer: &channelcore.inboundBuffer)
    }

    private func readFromBuffer<T>(buffer: inout [NIOAny]) throws -> T? {
        if buffer.isEmpty {
            return nil
        }
        guard let t = buffer.removeFirst().tryAs(type: T.self) else {
            throw EmbeddedChannelReadInboundError.wrongType
        }
        return t
    }
}

(uncompiled)

[ianpartridge]

Any thoughts on what you'd like to do with this?

[Lukasa]

So, my inclination is that we should deprecate the old functionality and simply replace it with new, better behaviour. That allows us to defer the SemVer major function removal. Then we can use @ianpartridge's solution (though the separate internal private function is probably unnecessary).

[weissi]

@ianpartridge now we could take this for NIO 2. Let's just add the throws.

[ianpartridge]

🙂 Let me page all this back in...

You'd like to change the existing function?

From:
public func readInbound<T>(as type: T.Type = T.self) -> T?
To:
public func readInbound<T>(as type: T.Type = T.self) throws -> T

[weissi]

@ianpartridge I think throws -> T?, ie. if wrong type: throws, if empty: nil

[weissi]

and then we can use

XCTAssertNoThrow(XCTAssertEqual(7, try channel.readInbound(as: Int.self)))

[ianpartridge]

OK, I guess make readOutbound() throwing too? I started to do this but there are a lot of test updates needed once the APIs throw. I'll try and find time to update this PR but I'm a bit limited on available time I'm afraid.

[weissi]

@ianpartridge I created #861 which should do all the test changes necessary, so after that, yours should just work pretty much as is

[ianpartridge]

Oh wow, thanks! It looks good to me.

[weissi]

@ianpartridge my PR is now merged, so you can rebase this one I think

[ianpartridge]

Thanks, I will. What are we going to throw? Are we going to add

public enum EmbeddedChannelReadInboundError: Error {
    case wrongType
}

[weissi]

Thanks, I will. What are we going to throw? Are we going to add

public enum EmbeddedChannelReadInboundError: Error {
    case wrongType
}

I think we could even nest that type into EmbeddedChannel, ie.

extension EmbeddedChannel {
    public enum Error {
        case wrongType(expected: Any.Type, actual: Any.Type)
    }
}

or so? Or given that so far it's only one error and we can't extend anyway after NIO 2

extension EmbeddedChannel {
    public struct WrongTypeError {
        public init (expected: Any.Type, actual: Any.Type) {}
    }
}

@Lukasa wdyt?

[Lukasa]

Certainly think we should embed it. Also a struct seems like the right thing to do too.

[ianpartridge]

WIP but I've pushed a commit with what I think you meant :)

[weissi]

@ianpartridge thanks, that looks great to me! Mind writing a test or two in EmbeddedChannelTests that test that we actually throw the right error with the right values?

[ianpartridge]

I pushed a few tests - let me know if you had something else in mind.

[ianpartridge]

Updated to conform WrongTypeError to Equatable, so the tests can check the contents exactly. It actually found a bug, the actual was being stored as NIOAny not the actual type.

[Lukasa]

@swift-nio-bot test this please

[weissi]

thanks very much @ianpartridge

[weissi]

@swift-nio-bot test this please