CombineCommunity / CombineExt

CombineExt provides a collection of operators, publishers and utilities for Combine, that are not provided by Apple themselves, but are common in other Reactive Frameworks and standards.
https://combine.community
MIT License
1.72k stars 151 forks source link

Add `mapToVoid` operator #129

Closed danshevluk closed 2 years ago

danshevluk commented 2 years ago

After fixing a mapToValue operator I got an idea to add mapToVoid operator. It's the most common use case for mapping publisher to a constant value. For example if you subscribe to NotificationCenter publisher and do not care about the value it produces:

NotificationCenter.default.publisher(for: UIApplication.userDidTakeScreenshotNotification)
    .mapToVoid()
   .sink {
       // Do something
   }
freak4pc commented 2 years ago

Hey, thanks for the idea ! I'm not entirely sure this operator is worthwhile.

map { _ in } mapToVoid() mapToValue(())

We're not really saving so much letter count with this very specific method. If there's more demand for it we can happily add it, but otherwise It seems like something that might be better off as a specific extension to some codebase.

WDYT?

danshevluk commented 2 years ago

@freak4pc

I had something similar in most projects that I worked on, so it feels like a common thing to use. I agree that it's not saving much letter space, but the main purpose of this extension is to explicitly mark muting the upstream.

Practical example. Imagine that you want to refresh some app data in one of 4 cases:

Here is how I would implement this using mapToVoid(). Looks cleaner than with .map { _ in } or mapToValue(())

Publishers.Merge4(
   $addressInfo.removeDuplicates().mapToVoid(),
   refreshControlPublisher.mapToVoid(),
   refreshTimerPublisher.mapToVoid(),
   NotificationCenter.default.publisher(for: UIApplication.didBecomeActiveNotification).mapToVoid()
)
// Published value type here is Void
.throttle(for: 4, scheduler: DispatchQueue.main, latest: true)
.sink {
   // schedule app data refresh
}
.store(in: &cancellables)

Is's more of a convenience method, but I feel like it's a good fit for CombineExt. Your call! 😃

codecov[bot] commented 2 years ago

Codecov Report

Merging #129 (aac9bde) into main (38a4d4c) will increase coverage by 0.04%. The diff coverage is 94.59%.

@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
+ Coverage   95.50%   95.55%   +0.04%     
==========================================
  Files          68       68              
  Lines        3781     3866      +85     
==========================================
+ Hits         3611     3694      +83     
- Misses        170      172       +2     
Impacted Files Coverage Δ
Tests/MapToValueTests.swift 97.75% <94.28%> (-2.25%) :arrow_down:
Sources/Operators/MapToValue.swift 100.00% <100.00%> (ø)
Sources/Operators/ZipMany.swift 100.00% <0.00%> (ø)
Sources/Operators/CombineLatestMany.swift 100.00% <0.00%> (ø)
Tests/ZipManyTests.swift 97.20% <0.00%> (+0.18%) :arrow_up:
Tests/CombineLatestManyTests.swift 97.65% <0.00%> (+0.24%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 38a4d4c...aac9bde. Read the comment docs.

danshevluk commented 2 years ago

@freak4pc do you have any further suggestions? To me looks like it's good to merge, can you do that? 🙏