pointfreeco / swift-composable-architecture

A library for building applications in a consistent and understandable way, with composition, testing, and ergonomics in mind.
https://www.pointfree.co/collections/composable-architecture
MIT License
12.07k stars 1.41k forks source link

SyncUp tutorial fixes #3139

Closed dafurman closed 2 months ago

dafurman commented 2 months ago

Here's a PR fixing a variety of issues I found while going through the tutorial. Overall it was quite good though!

mbrandonw commented 2 months ago

Hi @dafurman, thank you very much for taking the time to make these fixes to our tutorial! It is incredibly kind of you and we are very appreciative.

It is going to be a bit tough for us to make the time to review this PR, as is. Many of the changes are no-brainers, and we would happily pull them in, but others require us to actually go through the tutorial and make sure that it works as intended.

If you have a bit more time, you could separate out the simple changes from the complex ones in a separate PR, and that would allow us to merge the simple changes quickly, and make it easier for us to vet the complex ones later.

I think the simple ones include:

And if there is anything else you think is "simple" then please do include that.

If you were to PR those things we could happily merge, and then that might help make this PR a little smaller for us to grok.

dafurman commented 2 months ago

If you have a bit more time, you could separate out the simple changes from the complex ones in a separate PR, and that would allow us to merge the simple changes quickly, and make it easier for us to vet the complex ones later.

If you were to PR those things we could happily merge, and then that might help make this PR a little smaller for us to grok.

Sure that's completely understandable, I was a bit worried about the PR size myself.

I'll break this out into sensible chunks of complexity as you've suggested sometime this week!

mbrandonw commented 2 months ago

Hi @dafurman, we merged a bunch of tutorial fixes today and so you should be able to merge main into your branch to see what is left. Hopefully it isn't too much of a pain.

dafurman commented 2 months ago

@mbrandonw Alright, this should be ready for review now and be easier to look at this time around!

dafurman commented 2 months ago

@stephencelis

https://github.com/pointfreeco/swift-composable-architecture/pull/3139#discussion_r1631567790

Just curious was there actually a dismiss-related test failure in your runthrough? There used to be, but these days the test store automatically tears down effects when dismiss() is called by the reducer.

Sorry bout the delay - been busy lately. I think you're right though, because if I take the version of the test at step ImplementingTimer-04-code-0012.swift, I just get these 2 errors, instead of the 4 mentioned:

Screenshot 2024-06-13 at 12 12 15 AM

And then following the next step fixes that test, so this part of the tutorial could probably be shortened!