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

Tidied up some code using optional map #2549

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BenMaer
Copy link

@BenMaer BenMaer commented Oct 17, 2023

Simply shortens/tidies up a few places of code, each of which were 3 lines, but could be written with just one using Optional's map function.

This feels like too small of change to mention in the changelog. If you disagree, let me know and I can make a mention.

I was also thinking of further condensing this code to the existing extension on Disposables in AnonymousDisposable.swift:

extension Disposables {
.....
    /// Constructs a new disposable with the given action used for disposal. Provides an optional `dispose` param for convenience
    ///
    /// - parameter dispose: Optional disposal action which would be run upon calling `dispose`.
    public static func create(with dispose: (() -> Void)?) -> Disposable {
        return dispose.map({ create(with: $0) }) ?? create()
    }

}

That way, the three places I cleaned up wouldn't each be repeating the same logic. Let me know if that's something you'd want.

Copy link
Member

@freak4pc freak4pc left a comment

Choose a reason for hiding this comment

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

Nice fix! I added a clean-up suggestion.

RxSwift/Traits/PrimitiveSequence/Completable.swift Outdated Show resolved Hide resolved
} else {
disposable = Disposables.create()
}
let disposable: Disposable = onDisposed.map({ Disposables.create(with: $0) }) ?? Disposables.create()
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Author

Choose a reason for hiding this comment

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

Updated

} else {
disposable = Disposables.create()
}
let disposable: Disposable = onDisposed.map({ Disposables.create(with: $0) }) ?? Disposables.create()
Copy link
Member

Choose a reason for hiding this comment

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

ditto :)

Copy link
Author

Choose a reason for hiding this comment

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

Updated

@BenMaer
Copy link
Author

BenMaer commented Jan 4, 2024

Hey @freak4pc , I made the updates you suggested. Clever call with that!

Let me know if there's anything else I can do.

@freak4pc freak4pc force-pushed the main branch 2 times, most recently from 1198683 to 5949cbd Compare April 21, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants