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

Simplify MobiusLoop implementation #151

Merged
merged 6 commits into from
Apr 22, 2020

Conversation

JensAyton
Copy link
Contributor

@JensAyton JensAyton commented Apr 20, 2020

A complete rewrite of MobiusLoop, without EventProcessor.

After the threading rewrite, EventProcessor was just a wordy way to deal with initialization order and queue up effects received during setup. The latter can be handled by the WorkBag, and while the initialization dependencies are non-trivial, merging three layers of code into one makes things simpler.

Behaviour change: effects passed to the initializer or posted in event source/effect handler subscription are now handled in randomized order. Previously they were queued up deterministically in EventProcessor, which was an oversight.

@jeppes @pettermahlen

@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #151 into master will decrease coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #151      +/-   ##
==========================================
- Coverage   94.50%   94.39%   -0.11%     
==========================================
  Files          46       45       -1     
  Lines        1291     1249      -42     
==========================================
- Hits         1220     1179      -41     
+ Misses         71       70       -1     
Flag Coverage Δ
#ios 94.39% <100.00%> (-0.11%) ⬇️
#macspm 94.34% <100.00%> (-0.11%) ⬇️
Impacted Files Coverage Δ
MobiusCore/Source/Mobius.swift 97.18% <100.00%> (ø)
MobiusCore/Source/MobiusLoop.swift 100.00% <100.00%> (ø)
MobiusCore/Source/WorkBag.swift 100.00% <100.00%> (ø)

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 14df798...c80ffda. Read the comment docs.

Copy link
Contributor

@jeppes jeppes left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of comments

MobiusCore/Source/MobiusLoop.swift Outdated Show resolved Hide resolved
MobiusCore/Source/MobiusLoop.swift Show resolved Hide resolved
@JensAyton JensAyton force-pushed the mobius-loop-rewrite branch from 8907f3c to b61f4bc Compare April 20, 2020 12:40
@JensAyton
Copy link
Contributor Author

The redness of the build is from a non-mandatory codecov -0.13% delta

@JensAyton JensAyton force-pushed the mobius-loop-rewrite branch from df20f65 to 7bf37d7 Compare April 21, 2020 08:40
@JensAyton JensAyton merged commit 7c9bc24 into spotify:master Apr 22, 2020
@JensAyton JensAyton deleted the mobius-loop-rewrite branch April 22, 2020 09:46
@kmcbride kmcbride mentioned this pull request Oct 30, 2020
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.

3 participants