apple / swift-nio

Event-driven network application framework for high performance protocol servers & clients, non-blocking.
https://swiftpackageindex.com/apple/swift-nio/documentation
Apache License 2.0
7.93k stars 644 forks source link

collection of renames for 2.0 #663

Closed weissi closed 5 years ago

weissi commented 5 years ago

This is supposed to become a crowd-sourced random collection of renames in SwiftNIO for the 2.0 release. Please just add something like

weissi commented 5 years ago
weissi commented 5 years ago

again, CC'ing some folks @helje5 / @tanner0101 / @ianpartridge / @pushkarnk / @MrMage / @tachyonics / @airspeedswift / @moiseev / @kjessup / @normanmaurer / @Lukasa / @Joannis . Please name your small pet peeves here :)

MrMage commented 5 years ago

What’s the motivation for changing port numbers to Int? Aren’t they always unsigned 16-bit integers?

weissi commented 5 years ago
  • [x] rename ctx to context

What’s the motivation for changing port numbers to Int? Aren’t they always unsigned 16-bit integers?

it's Swift convention to always use Int for everything (at least in public API unless it really doesn't work, like nanoseconds timeouts need to be Int64 because 32 bit platforms) because it makes interoperability work much better. See for example Array's var count: Int even though clearly it's never negative...

MrMage commented 5 years ago

Makes sense, thanks!

On 27. Nov 2018, at 12:11, Johannes Weiss notifications@github.com wrote:

rename ctx to context What’s the motivation for changing port numbers to Int? Aren’t they always unsigned 16-bit integers?

it's Swift convention to always use Int for everything (at least in public API) because it makes interoperability work much better. See for example Array's var count: Int even though clearly it's never negative...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

weissi commented 5 years ago
ianpartridge commented 5 years ago

Do you want to tackle https://github.com/apple/swift-nio/pull/602 in 2.0 too?

weissi commented 5 years ago

@ianpartridge absolutely! thanks for reminding me

weissi commented 5 years ago

to match Result in the stdlib

tanner0101 commented 5 years ago
Lukasa commented 5 years ago

What does remove(before:) mean? I don’t think that one makes much sense.

tanner0101 commented 5 years ago

What does remove(before:) mean? I don’t think that one makes much sense.

Sorry, the comment should just say "add methods".

screen shot 2019-01-23 at 5 02 42 pm

Mostly I just think it would be nice to have the option to specify name / after / before when adding multiple handlers. For name, something like [String: ChannelHandler] would be nice. I also think naming the multiple add method add(handlers:) might be more Swifty.

weissi commented 5 years ago
  • [ ] rename thenThrowing to mapThrowing and add a thenThrowing that accepts a future.

strong -1 to any name that starts with map*, that name would be just wrong. map's signature is

(ELF<Value>) -> ((Value -> NewValue)) -> ELF<NewValue>

, the important part here is that you can only transform a success to a success and nothing else.

flatMapThrowing (used to be thenThrowing) is a good name because its name is exactly what it does: It takes a throwing thing, converts that to an ELF and then flatMaps that. Works. You just can't implement that function with a map only but you can with a flatMap.

Namely, there's a natural translation from

(A) throws -> B

to

(A) -> EventLoopFuture<B>

which is exactly the right input type for a flatMap. But there isn't any from

(A) throws -> B

to

(A) -> B

which would be the right input type for a map. I'm open to giving it some other name combineThrowing but it can't start with map because that's really wrong and misleading. My point is still that flatMapThrowing is arguably the best name because that's what it is (see it's implementation):

    public func flatMapThrowing<NewValue>(file: StaticString = #file,
                                line: UInt = #line,
                                _ callback: @escaping (Value) throws -> NewValue) -> EventLoopFuture<NewValue> {
        return self.flatMap(file: file, line: line) { (value: Value) -> EventLoopFuture<NewValue> in
            do {
                return EventLoopFuture<NewValue>(eventLoop: self.eventLoop, result: try callback(value), file: file, line: line)
            } catch {
                return EventLoopFuture<NewValue>(eventLoop: self.eventLoop, error: error, file: file, line: line)
            }
        }
    }

but I realise that people don't like it so I'd be open to combineThrowing or so.

  • [ ] add promise.succeed() with no args where Success == Void

-1 to this one too. Overloads always lead to sadness and this one saves two characters (#776 is the PR to get rid of the labels for succeed and fail).

  • [ ] add cascadeSuccess to future.

+1

weissi commented 5 years ago

closing because mostly done apart from the work already filed as #483