Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optional type parameter for newPromise() #671

Merged
merged 1 commit into from Nov 28, 2018

Conversation

weissi
Copy link
Member

@weissi weissi commented Nov 27, 2018

Motivation:

When creating new promises I always find it very frustrating to type the
: EventLoopPromise<Type> type annotation but it's necessary for the
compiler to know type the promise will be fulfilled with.

Modifications:

allow an optional for: SomeType.self parameter for newPromise as

let p = eventLoop.newPromise(for: Int.self)

is much easier to type than

let p: EventLoopPromise<Int> = eventLoop.newPromise()

Result:

easier to write code

@weissi weissi added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Nov 27, 2018
@weissi weissi added this to the 1.12.0 milestone Nov 28, 2018
@helje5
Copy link
Contributor

helje5 commented Nov 28, 2018

I know that some people in the server space do this, but I always found those type.self things particularly ugly and AFAIK it is not done anywhere else in Swift or Foundation itself. I think the expected API is more like:

let p = EventLoopPromise<Int>(in: eventLoop)

Which, funny enough, is what newPromise is doing. (The newPromise thing looks like one of the Java'ish factory patterns you find a lot in NIO.)

@Lukasa
Copy link
Contributor

Lukasa commented Nov 28, 2018

I know that some people in the server space do this, but I always found those type.self things particularly ugly and AFAIK it is not done anywhere else in Swift or Foundation itself.

Sure it is. Codable uses it heavily, both in the API (Decoder.container(keyedBy:)) and in the implementation (check out all these decode functions in JSONEncoder.swift). It's used in String (String.decoding(codeUnits:as:)), it's used in the unsafe pointer types (see UnsafeRawBufferPointer.load(fromByteOffset:as:) and UnsafeRawBufferPointer.storeBytes(of:toByteOffset:as:) as examples).

And in fact the last time we did something like this (in 94c21b6) it was at the suggestion of some members of the standard library team.

TL;DR: the prior art for .Type here is pretty good.

The concern about the factory method is reasonable, but as this is intended as a semver minor we can't fix it here. I'm +1 on the idea of changing to a construct for SwiftNIO 2.0 though.

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a note in the diff about the naming, which I don't love.

To @helje5's point though, if you and or @normanmaurer are interested in changing the way we construct EventLoopPromise objects for SwiftNIO 2, I might suggest deferring this change until then.

@@ -318,7 +318,7 @@ extension EventLoop {
}

/// Creates and returns a new `EventLoopPromise` that will be notified using this `EventLoop` as execution `Thread`.
public func newPromise<T>(file: StaticString = #file, line: UInt = #line) -> EventLoopPromise<T> {
public func newPromise<T>(type: T.Type = T.self, file: StaticString = #file, line: UInt = #line) -> EventLoopPromise<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the name type here. It sits weirdly with me: it's not the type of the promise, it's the type of what it contains. Not sure what the better name choice is though: resultType, maybe? Or futureResult? Or producing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lukasa makePromise<T>(for type: T.Type = T.self) ? that reads okay: 'make promise for integer'.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, in 1.0 obviously newPromise

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lukasa updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would makePromise<T>(of type: T.Type = T.self) be better?

Copy link
Member

@ktoso ktoso Nov 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems I'm also late to the party here, and wanted to suggest of as well when I was reading the PR. "a promise of string (in the future)" somehow reads better than "a promise for string (in the future)". Your call tho.

// Overall +1 though! :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can still change it, it's not released. Let's page stdlib folk for API guidance, CC @airspeedswift / @lorentey

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, we now landed on newPromise(of: T.Type = T.self) because 'making a promise of a type' is more grammatical than 'making a promise for a type' --> #672

@weissi
Copy link
Member Author

weissi commented Nov 28, 2018

@Lukasa / @normanmaurer I'm open to opening up EventLoopPromise's init and making it public but I'm against removing the factory from the event loop. I think we can have both.

@Lukasa
Copy link
Contributor

Lukasa commented Nov 28, 2018

I'm happy enough with that as a compromise position.

Motivation:

When creating new promises I always find it very frustrating to type the
`: EventLoopPromise<Type>` type annotation but it's necessary for the
compiler to know type the promise will be fulfilled with.

Modifications:

allow an optional `for: SomeType.self` parameter for `newPromise` as

    let p = eventLoop.newPromise(for: Int.self)

is much easier to type than

    let p: EventLoopPromise<Int> = eventLoop.newPromise()

Result:

easier to write code
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed this the way I know best: by looking at the real change, and then trusting the compiler will punish you if you screwed any of the rest of it up.

Uh.

I mean.

+1 LGTM.

@Lukasa Lukasa merged commit c1a84a0 into apple:master Nov 28, 2018
@weissi weissi deleted the jw-new-promise-type branch November 28, 2018 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants