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

standardize `ELF.cascade*` collection of methods #802

Merged
merged 2 commits into from Feb 5, 2019

Conversation

Projects
None yet
3 participants
@Mordil
Copy link
Contributor

Mordil commented Feb 2, 2019

Motivation:

The ELF.cascade methods have a parameter label promise that does not match Swift API Guidelines, and a way to cascade just successes is not available - while for failures there is.

Modifications:

ELF.cascade* methods that already exist have had their promise label renamed to to, and a new ELF.cascadeSuccess method has been added.

Result:

EventLoopFuture now has the cascade methods ELF.cascade(to:), ELF.cascadeFailure(to:), and ELF.cascadeSuccess(to:)

@swift-nio-bot

This comment has been minimized.

Copy link

swift-nio-bot commented Feb 2, 2019

Can one of the admins verify this patch?

3 similar comments
@swift-nio-bot

This comment has been minimized.

Copy link

swift-nio-bot commented Feb 2, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

swift-nio-bot commented Feb 2, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

swift-nio-bot commented Feb 2, 2019

Can one of the admins verify this patch?

@Mordil Mordil changed the title Cascade Standardize EventLoopFuture.cascade method collection Feb 2, 2019

@Mordil Mordil changed the title Standardize EventLoopFuture.cascade method collection Standardize ELF cascade method collection Feb 2, 2019

@Lukasa
Copy link
Contributor

Lukasa left a comment

Thanks for this! Generally looks good, but I've left some notes in the diff. Can you also use the commit message template? Thanks!

Show resolved Hide resolved Sources/NIO/EventLoopFuture.swift
Show resolved Hide resolved Sources/NIO/EventLoopFuture.swift Outdated
@Mordil

This comment has been minimized.

Copy link
Contributor Author

Mordil commented Feb 4, 2019

@Lukasa Should the commit message template be used in the PR's description & title, or should I squash and update the commit message directly in git?

@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Feb 4, 2019

Squash and update in Git, please :)

@Mordil Mordil force-pushed the Mordil:cascade branch from 004b7ac to 96801f5 Feb 4, 2019

@Mordil Mordil changed the title Standardize ELF cascade method collection standardize `ELF.cascade*` collection of methods Feb 4, 2019

@Mordil

This comment has been minimized.

Copy link
Contributor Author

Mordil commented Feb 4, 2019

Done - this should be good for final review :)

@Lukasa
Copy link
Contributor

Lukasa left a comment

One tiny note!

Show resolved Hide resolved docs/public-api-changes-NIO1-to-NIO2.md
standardize `ELF.cascade*` collection of methods
Motivation:

The `ELF.cascade` methods have a parameter label `promise` that does not match Swift API Guidelines, and a way to cascade just successes is not available - while for failures there is.

Modifications:

`ELF.cascade*` methods that already exist have had their `promise` label renamed to `to`, and a new `ELF.cascadeSuccess` method has been added.

Result:

EventLoopFuture now has the cascade methods `ELF.cascade(to:)`, `ELF.cascadeFailure(to:)`, and `ELF.cascadeSuccess(to:)`

@Mordil Mordil force-pushed the Mordil:cascade branch from 96801f5 to 40661d6 Feb 4, 2019

@Lukasa

Lukasa approved these changes Feb 4, 2019

Copy link
Contributor

Lukasa left a comment

Fab, ok. Same note here: I'm going to merge this, but @weissi may want to provide post-hoc changes based on his code review when he's back.

@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Feb 4, 2019

@swift-nio-bot test this please

@Lukasa Lukasa added this to the 2.0.0 milestone Feb 4, 2019

@Lukasa

This comment has been minimized.

Copy link
Contributor

Lukasa commented Feb 4, 2019

@swift-nio-bot test this please

@Lukasa Lukasa merged commit caf9a3d into apple:master Feb 5, 2019

1 check passed

pull request validation (5.0) Build finished.
Details

@Mordil Mordil deleted the Mordil:cascade branch Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment