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

Add mapToVoid operator #129

Merged
merged 2 commits into from
Jul 23, 2022
Merged

Conversation

danshevluk
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

@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:

  • data model updated
  • refresh control triggered
  • if some timer passed
  • if the app became active

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
Copy link

codecov bot commented May 25, 2022

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%) ⬇️
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%) ⬆️
Tests/CombineLatestManyTests.swift 97.65% <0.00%> (+0.24%) ⬆️

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.

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.

I'll go for it :) one fix

Co-authored-by: Shai Mishali <freak4pc@gmail.com>
@danshevluk
Copy link
Contributor Author

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

@freak4pc freak4pc merged commit 5c17b57 into CombineCommunity:main Jul 23, 2022
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.

2 participants