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

Rename internal `flatMap` to `tryMap` #2892

Merged
merged 1 commit into from Aug 11, 2019

Conversation

@philtre
Copy link
Contributor

commented Aug 7, 2019

Goals ⚽️

  1. Rename internal Result mapping functions:
  • flatMap to tryMap,
  • flatMapError to tryMapError
  1. Rename internal Result convenience properties:
  • success to value
  • failure to error
  1. Use autoclosure properties in convenience Result initializer

This PR builds on top of #2891
and is an attempt to break up #2864

@jshier
Copy link
Contributor

left a comment

We can't use value / error for the Result extensions, it causes confusing messages when users try to use the properties from their code. Otherwise I think this looks okay for now, but I'll need to review again after the first PR is merged.

extension Result {
/// Returns the associated value if the result is a success, `nil` otherwise.
var value: Success? {

This comment has been minimized.

Copy link
@jshier

jshier Aug 7, 2019

Contributor

These property names should not be changed. In fact, they were changed to success / failure to get away from our previous names in order to prevent a confusing error seen when using value / error with older Alamofire tutorials.

This comment has been minimized.

Copy link
@philtre

philtre Aug 7, 2019

Author Contributor

I was unaware of this problem. I will revert this change.

/// - Parameters:
/// - value: A value.
/// - error: An `Error`.
init(value: @autoclosure () -> Success, error: @autoclosure () -> Failure?) {

This comment has been minimized.

Copy link
@jshier

jshier Aug 7, 2019

Contributor

What was the purpose of this change? I can't find a use site in the current diff that shows improvement with the use of autoclosures..

This comment has been minimized.

Copy link
@philtre

philtre Aug 7, 2019

Author Contributor

If error is non-nil, the value closure isn't executed at all. I can also revert this change if you disagree that there is any benefit in this.

@philtre philtre force-pushed the philtre:feature/modify-result-extensions branch 2 times, most recently from 7cad1cf to de3784d Aug 7, 2019

@philtre philtre changed the title Rename internal Result convenience properties and functions Rename internal `flatMap` to `tryMap` Aug 7, 2019

@jshier jshier changed the base branch from master to usage-documentation-update Aug 9, 2019

@jshier jshier changed the base branch from usage-documentation-update to master Aug 9, 2019

@jshier

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

@philtre Can you merge master or rebase to fix the conflicts and cleanup the diff here?

@philtre philtre force-pushed the philtre:feature/modify-result-extensions branch from de3784d to c89bb6a Aug 9, 2019

@philtre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Branch rebased.

@jshier
jshier approved these changes Aug 9, 2019
Copy link
Contributor

left a comment

👍 Looks good!

@cnoon Anything to add?

@jshier jshier merged commit 5e3e905 into Alamofire:master Aug 11, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@jshier jshier added this to the 5.0.0.rc.1 milestone Aug 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.