-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
SyncUp tutorial fixes #3139
SyncUp tutorial fixes #3139
Conversation
...cture/Documentation.docc/Tutorials/BuildingSyncUps/02-ListsOfSyncUps/ListsOfSyncUps.tutorial
Outdated
Show resolved
Hide resolved
@@ -39,7 +39,12 @@ struct SyncUpForm { | |||
guard | |||
!state.syncUp.attendees.isEmpty, | |||
let firstIndex = indices.first | |||
else { return .none } | |||
else { | |||
state.syncUp.attendees.append( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure that there's always an attendee, so that
var durationPerAttendee: Duration {
duration / attendees.count
}
doesn't crash
@@ -5,7 +5,7 @@ import XCTest | |||
|
|||
class SyncUpFormTests: XCTestCase { | |||
@MainActor | |||
func testRemoveAttendee() async { | |||
func testRemoveFocusedAttendee() async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that the testRemoveAttendee()
function fails, rather than passes as the instructions state.
The pathway to fixing this pretty much comes down to a lot of the same logic in testRemoveFocusedAttendee
, so in this PR I suggest just dropping testRemoveAttendee()
in this tutorial and only focusing on testRemoveFocusedAttendee()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up reverting changes here, to make this PR easier to review.
If you'd like, I can do a followup PR to make the changes I've mentioned here, once this PR is merged, or if you want to handle it in your own way that's cool too.
...Documentation.docc/Tutorials/BuildingSyncUps/05-PersistingSyncUps/PersistingSyncUps.tutorial
Outdated
Show resolved
Hide resolved
...cumentation.docc/Tutorials/BuildingSyncUps/06-SyncUpDetail/EditingAndDeletingSyncUp.tutorial
Outdated
Show resolved
Hide resolved
...occ/Tutorials/BuildingSyncUps/07-SyncUpDetailNavigation/MeetingNavigation-01-code-0003.swift
Outdated
Show resolved
Hide resolved
@@ -2,7 +2,7 @@ import ComposableArchitecture | |||
import SwiftUI | |||
|
|||
@Reducer | |||
struct App { | |||
struct AppFeature { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid conflicting with the SwiftUI.App
protocol, I switched mention of this to AppFeature
. There were a couple times where this was referenced as AppFeature
in the tutorial anyways, so it seemed like the right name.
struct App { | ||
@Reducer | ||
struct AppFeature { | ||
@Reducer(state: .equatable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Necessary for compilation, and I added a note on this in the tutorial where this gets added.
@@ -18,7 +18,9 @@ | |||
> Note: You may already have an App.swift in your project that holds the entry point of | |||
the application (i.e. `@main`). If so, you can name this file AppFeature.swift, or you can | |||
rename the entry point to Main.swift. | |||
|
|||
> | |||
> We often leave out suffixing reducers with names like "feature" or "reducer", but in this case we must avoid conflicting with SwiftUI's [App](https://developer.apple.com/documentation/swiftui/app) protocol, so `AppFeature` is used as the type name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured I'd toss this small blurb in here too, but it can be removed if you want.
@@ -24,6 +24,7 @@ final class RecordMeetingTests: XCTestCase { | |||
$0.continuousClock = clock | |||
$0.date.now = Date(timeIntervalSince1970: 1234567890) | |||
$0.uuid = .incrementing | |||
$0.dismiss = DismissEffect {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applies the most basic override before we give it something better in the next step, per
Override the
dismiss
dependency in thewithDependencies
trailer closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
} | ||
|
||
extension PersistenceReaderKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes regarding these files add one more step and some tutorial commentary to go from
extension URL {
static let syncUps = Self.documentsDirectory.appending(component: "sync-ups.json")
}
to
extension PersistenceReaderKey
where Self == PersistenceKeyDefault<FileStorageKey<IdentifiedArrayOf<SyncUp>>> {
static var syncUps: Self {
PersistenceKeyDefault(
.fileStorage(.documentsDirectory.appending(component: "sync-ups.json")),
[]
)
}
}
In the current version of the tutorial, later on, this shorthand version is used, so we might as well just standardize on this throughout the tutorial, and this ensures that later parts of the tutorial compile.
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. |
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! |
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. |
@mbrandonw Alright, this should be ready for review now and be easier to look at this time around! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I went through everything and they're all great improvements.
I pushed a few small things and am gonna merge as-is, but if you have a project in a handy state, I had a question.
@@ -24,6 +24,7 @@ final class RecordMeetingTests: XCTestCase { | |||
$0.continuousClock = clock | |||
$0.date.now = Date(timeIntervalSince1970: 1234567890) | |||
$0.uuid = .incrementing | |||
$0.dismiss = DismissEffect {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
…ure to from: "1.11.2" (#1137) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [pointfreeco/swift-composable-architecture](https://togithub.com/pointfreeco/swift-composable-architecture) | minor | `from: "1.10.4"` -> `from: "1.11.2"` | --- ### Release Notes <details> <summary>pointfreeco/swift-composable-architecture (pointfreeco/swift-composable-architecture)</summary> ### [`v1.11.2`](https://togithub.com/pointfreeco/swift-composable-architecture/releases/tag/1.11.2) [Compare Source](https://togithub.com/pointfreeco/swift-composable-architecture/compare/1.11.1...1.11.2) #### What's Changed - Fixed: Avoid potential sendability warnings in Swift 6 mode ([https://github.com/pointfreeco/swift-composable-architecture/pull/3167](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3167)). - Fixed: `PersistenceKeyDefault` no longer uses the loaded value as an initial value (thanks [@​fdzsergio](https://togithub.com/fdzsergio), [https://github.com/pointfreeco/swift-composable-architecture/pull/3174](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3174)). - Fixed: Address a potential deadlock by isolating `Shared.withLock` to the main actor ([https://github.com/pointfreeco/swift-composable-architecture/pull/3178](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3178)). - Fixed: Disfavor `Shared`'s optional dynamic member lookup ([https://github.com/pointfreeco/swift-composable-architecture/pull/3170](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3170)). Note that this fix may be source breaking. See the [migration guide](https://pointfreeco.github.io/swift-composable-architecture/main/documentation/composablearchitecture/migratingto1.11) for more details. - Fixed: Don't over-observe app storage mutations ([https://github.com/pointfreeco/swift-composable-architecture/pull/3186](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3186)). - Fixed: `$shared.elements` is now stable based on identity, and restricted to identified arrays ([https://github.com/pointfreeco/swift-composable-architecture/pull/3187](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3187)). - Infrastructure: Drop Swift <5.9 support ([https://github.com/pointfreeco/swift-composable-architecture/pull/3185](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3185)). Xcode 15 has been required for app submission since April, so we can keep our Swift support in line with Apple's. - Infrastructure: 1.11 migration guide fixes (thanks [@​larryonoff](https://togithub.com/larryonoff), [https://github.com/pointfreeco/swift-composable-architecture/pull/3184](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3184)); tutorial typo fixes (thanks [@​meltsplit](https://togithub.com/meltsplit), [https://github.com/pointfreeco/swift-composable-architecture/pull/3161](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3161)). #### New Contributors - [@​meltsplit](https://togithub.com/meltsplit) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3161](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3161) - [@​fdzsergio](https://togithub.com/fdzsergio) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3174](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3174) **Full Changelog**: pointfreeco/swift-composable-architecture@1.11.1...1.11.2 ### [`v1.11.1`](https://togithub.com/pointfreeco/swift-composable-architecture/releases/tag/1.11.1) [Compare Source](https://togithub.com/pointfreeco/swift-composable-architecture/compare/1.11.0...1.11.1) #### What's Changed - Fixed: Support swift-syntax from 600.0.0-latest ([https://github.com/pointfreeco/swift-composable-architecture/pull/3160](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3160)). - Fixed: `Shared.withLock` now pass values by continuation ([https://github.com/pointfreeco/swift-composable-architecture/pull/3154](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3154)). - Infrastructure: Clean up DocC and link to new migration guide in README ([https://github.com/pointfreeco/swift-composable-architecture/pull/3153](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3153)); SyncUp tutorial fixes (thanks [@​dafurman](https://togithub.com/dafurman), [https://github.com/pointfreeco/swift-composable-architecture/pull/3139](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3139); thanks [@​RuiAAPeres](https://togithub.com/RuiAAPeres), [https://github.com/pointfreeco/swift-composable-architecture/pull/3159](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3159)); note Swift bug in documentation ([https://github.com/pointfreeco/swift-composable-architecture/pull/3157](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3157)). #### New Contributors - [@​RuiAAPeres](https://togithub.com/RuiAAPeres) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3159](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3159) **Full Changelog**: pointfreeco/swift-composable-architecture@1.11.0...1.11.1 ### [`v1.11.0`](https://togithub.com/pointfreeco/swift-composable-architecture/releases/tag/1.11.0) [Compare Source](https://togithub.com/pointfreeco/swift-composable-architecture/compare/1.10.4...1.11.0) #### What's Changed - Added: `Shared.withLock`, for mutating shared state from asynchronous contexts ([https://github.com/pointfreeco/swift-composable-architecture/pull/3136](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3136)). Direct mutations from asynchronous contexts is marked unavailable and will be an error in Swift 6. - Added: `SharedReader.constant` ([https://github.com/pointfreeco/swift-composable-architecture/pull/3127](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3127)). - Added: `$store.scope` will now emit a warning when a dismiss action doesn't `nil` out a child feature, suggesting a `Reducer.ifLet` (or parent integration) is missing ([https://github.com/pointfreeco/swift-composable-architecture/pull/3089](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3089)). - Deprecated: `Shared`'s optional dynamic member lookup overload has been deprecated in favor of a `Binding.init` that unwraps optional values ([https://github.com/pointfreeco/swift-composable-architecture/pull/3145](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3145)). - Fixed: Avoid crash when using `.appStorage` with a `URL` value (thanks [@​pwszebor](https://togithub.com/pwszebor), [https://github.com/pointfreeco/swift-composable-architecture/pull/3098](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3098)). - Fixed: Worked around a build failure when integrating with Tuist ([https://github.com/pointfreeco/swift-composable-architecture/pull/3140](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3140)). - Infrastructure: Tutorial fixes ([https://github.com/pointfreeco/swift-composable-architecture/pull/3076](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3076); thanks [@​MartinMoizard](https://togithub.com/MartinMoizard), [https://github.com/pointfreeco/swift-composable-architecture/pull/3078](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3078); [https://github.com/pointfreeco/swift-composable-architecture/pull/3072](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3072); thanks [@​hmhv](https://togithub.com/hmhv), [https://github.com/pointfreeco/swift-composable-architecture/pull/3091](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3091); thanks [@​gibachan](https://togithub.com/gibachan), [https://github.com/pointfreeco/swift-composable-architecture/pull/3099](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3099); thanks [@​btr-better](https://togithub.com/btr-better), [https://github.com/pointfreeco/swift-composable-architecture/pull/3107](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3107); thanks [@​woxtu](https://togithub.com/woxtu), [https://github.com/pointfreeco/swift-composable-architecture/pull/3119](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3119), [https://github.com/pointfreeco/swift-composable-architecture/pull/3123](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3123); [https://github.com/pointfreeco/swift-composable-architecture/pull/3135](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3135); [https://github.com/pointfreeco/swift-composable-architecture/pull/3141](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3141); [https://github.com/pointfreeco/swift-composable-architecture/pull/3148](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3148)); DocC fixes ([https://github.com/pointfreeco/swift-composable-architecture/pull/3085](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3085); [https://github.com/pointfreeco/swift-composable-architecture/pull/3087](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3087); thanks [@​JOyo246](https://togithub.com/JOyo246), [https://github.com/pointfreeco/swift-composable-architecture/pull/3092](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3092); thanks [@​leeari95](https://togithub.com/leeari95), [https://github.com/pointfreeco/swift-composable-architecture/pull/3110](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3110); [https://github.com/pointfreeco/swift-composable-architecture/pull/3138](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3138)); README fixes (thanks [@​Matt54](https://togithub.com/Matt54), [https://github.com/pointfreeco/swift-composable-architecture/pull/3129](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3129)); expose some navigation APIs with `@_spi(Internal)` (thanks [@​Alex293](https://togithub.com/Alex293), [https://github.com/pointfreeco/swift-composable-architecture/pull/3097](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3097)). #### New Contributors - [@​MartinMoizard](https://togithub.com/MartinMoizard) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3078](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3078) - [@​JOyo246](https://togithub.com/JOyo246) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3092](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3092) - [@​pwszebor](https://togithub.com/pwszebor) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3098](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3098) - [@​Alex293](https://togithub.com/Alex293) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3097](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3097) - [@​gibachan](https://togithub.com/gibachan) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3099](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3099) - [@​leeari95](https://togithub.com/leeari95) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3110](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3110) - [@​btr-better](https://togithub.com/btr-better) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3107](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3107) - [@​Matt54](https://togithub.com/Matt54) made their first contribution in [https://github.com/pointfreeco/swift-composable-architecture/pull/3129](https://togithub.com/pointfreeco/swift-composable-architecture/pull/3129) **Full Changelog**: pointfreeco/swift-composable-architecture@1.10.4...1.11.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDkuNCIsInVwZGF0ZWRJblZlciI6IjM2LjEwOS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
Here's a PR fixing a variety of issues I found while going through the tutorial. Overall it was quite good though!