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

SyncUp tutorial fixes #3139

Merged
merged 34 commits into from
Jun 7, 2024
Merged

Conversation

dafurman
Copy link
Contributor

@dafurman dafurman commented Jun 4, 2024

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

@@ -39,7 +39,12 @@ struct SyncUpForm {
guard
!state.syncUp.attendees.isEmpty,
let firstIndex = indices.first
else { return .none }
else {
state.syncUp.attendees.append(
Copy link
Contributor Author

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

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().

Screenshot 2024-06-03 at 7 54 01 PM Screenshot 2024-06-03 at 7 52 44 PM

Copy link
Contributor Author

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.

@@ -2,7 +2,7 @@ import ComposableArchitecture
import SwiftUI

@Reducer
struct App {
struct AppFeature {
Copy link
Contributor Author

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

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

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

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 the withDependencies trailer closure.

Copy link
Member

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.

@dafurman dafurman marked this pull request as ready for review June 4, 2024 03:19
}
}

extension PersistenceReaderKey
Copy link
Contributor Author

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.

@mbrandonw
Copy link
Member

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:

  • Simple typos
  • Poorly formatted code like the {@MainActor stuff in tests
  • Obviously missing code like durationPerAttendee
  • And you can delete the commented out code

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

dafurman commented Jun 4, 2024

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

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

dafurman commented Jun 7, 2024

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

Copy link
Member

@stephencelis stephencelis left a 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 {}
Copy link
Member

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.

@stephencelis stephencelis merged commit 8631b5f into pointfreeco:main Jun 7, 2024
7 checks passed
@dafurman
Copy link
Contributor Author

dafurman commented Jun 13, 2024

@stephencelis

#3139 (comment)

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!

cgrindel-self-hosted-renovate bot added a commit to cgrindel/rules_swift_package_manager that referenced this pull request Jul 2, 2024
…ure to from: "1.11.2" (#1137)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[pointfreeco/swift-composable-architecture](https://github.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://github.com/pointfreeco/swift-composable-architecture/releases/tag/1.11.2)

[Compare
Source](https://github.com/pointfreeco/swift-composable-architecture/compare/1.11.1...1.11.2)

#### What's Changed

- Fixed: Avoid potential sendability warnings in Swift 6 mode
([pointfreeco/swift-composable-architecture#3167).
- Fixed: `PersistenceKeyDefault` no longer uses the loaded value as an
initial value (thanks
[@&#8203;fdzsergio](https://github.com/fdzsergio),
[pointfreeco/swift-composable-architecture#3174).
- Fixed: Address a potential deadlock by isolating `Shared.withLock` to
the main actor
([pointfreeco/swift-composable-architecture#3178).
- Fixed: Disfavor `Shared`'s optional dynamic member lookup
([pointfreeco/swift-composable-architecture#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
([pointfreeco/swift-composable-architecture#3186).
- Fixed: `$shared.elements` is now stable based on identity, and
restricted to identified arrays
([pointfreeco/swift-composable-architecture#3187).
- Infrastructure: Drop Swift <5.9 support
([pointfreeco/swift-composable-architecture#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
[@&#8203;larryonoff](https://github.com/larryonoff),
[pointfreeco/swift-composable-architecture#3184);
tutorial typo fixes (thanks
[@&#8203;meltsplit](https://github.com/meltsplit),
[pointfreeco/swift-composable-architecture#3161).

#### New Contributors

- [@&#8203;meltsplit](https://github.com/meltsplit) made their first
contribution in
[pointfreeco/swift-composable-architecture#3161
- [@&#8203;fdzsergio](https://github.com/fdzsergio) made their first
contribution in
[pointfreeco/swift-composable-architecture#3174

**Full Changelog**:
pointfreeco/swift-composable-architecture@1.11.1...1.11.2

###
[`v1.11.1`](https://github.com/pointfreeco/swift-composable-architecture/releases/tag/1.11.1)

[Compare
Source](https://github.com/pointfreeco/swift-composable-architecture/compare/1.11.0...1.11.1)

#### What's Changed

- Fixed: Support swift-syntax from 600.0.0-latest
([pointfreeco/swift-composable-architecture#3160).
- Fixed: `Shared.withLock` now pass values by continuation
([pointfreeco/swift-composable-architecture#3154).
- Infrastructure: Clean up DocC and link to new migration guide in
README
([pointfreeco/swift-composable-architecture#3153);
SyncUp tutorial fixes (thanks
[@&#8203;dafurman](https://github.com/dafurman),
[pointfreeco/swift-composable-architecture#3139;
thanks [@&#8203;RuiAAPeres](https://github.com/RuiAAPeres),
[pointfreeco/swift-composable-architecture#3159);
note Swift bug in documentation
([pointfreeco/swift-composable-architecture#3157).

#### New Contributors

- [@&#8203;RuiAAPeres](https://github.com/RuiAAPeres) made their first
contribution in
[pointfreeco/swift-composable-architecture#3159

**Full Changelog**:
pointfreeco/swift-composable-architecture@1.11.0...1.11.1

###
[`v1.11.0`](https://github.com/pointfreeco/swift-composable-architecture/releases/tag/1.11.0)

[Compare
Source](https://github.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
([pointfreeco/swift-composable-architecture#3136).
Direct mutations from asynchronous contexts is marked unavailable and
will be an error in Swift 6.
- Added: `SharedReader.constant`
([pointfreeco/swift-composable-architecture#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
([pointfreeco/swift-composable-architecture#3089).
- Deprecated: `Shared`'s optional dynamic member lookup overload has
been deprecated in favor of a `Binding.init` that unwraps optional
values
([pointfreeco/swift-composable-architecture#3145).
- Fixed: Avoid crash when using `.appStorage` with a `URL` value (thanks
[@&#8203;pwszebor](https://github.com/pwszebor),
[pointfreeco/swift-composable-architecture#3098).
- Fixed: Worked around a build failure when integrating with Tuist
([pointfreeco/swift-composable-architecture#3140).
- Infrastructure: Tutorial fixes
([pointfreeco/swift-composable-architecture#3076;
thanks [@&#8203;MartinMoizard](https://github.com/MartinMoizard),
[pointfreeco/swift-composable-architecture#3078;
[pointfreeco/swift-composable-architecture#3072;
thanks [@&#8203;hmhv](https://github.com/hmhv),
[pointfreeco/swift-composable-architecture#3091;
thanks [@&#8203;gibachan](https://github.com/gibachan),
[pointfreeco/swift-composable-architecture#3099;
thanks [@&#8203;btr-better](https://github.com/btr-better),
[pointfreeco/swift-composable-architecture#3107;
thanks [@&#8203;woxtu](https://github.com/woxtu),
[pointfreeco/swift-composable-architecture#3119,
[pointfreeco/swift-composable-architecture#3123;
[pointfreeco/swift-composable-architecture#3135;
[pointfreeco/swift-composable-architecture#3141;
[pointfreeco/swift-composable-architecture#3148);
DocC fixes
([pointfreeco/swift-composable-architecture#3085;
[pointfreeco/swift-composable-architecture#3087;
thanks [@&#8203;JOyo246](https://github.com/JOyo246),
[pointfreeco/swift-composable-architecture#3092;
thanks [@&#8203;leeari95](https://github.com/leeari95),
[pointfreeco/swift-composable-architecture#3110;
[pointfreeco/swift-composable-architecture#3138);
README fixes (thanks [@&#8203;Matt54](https://github.com/Matt54),
[pointfreeco/swift-composable-architecture#3129);
expose some navigation APIs with `@_spi(Internal)` (thanks
[@&#8203;Alex293](https://github.com/Alex293),
[pointfreeco/swift-composable-architecture#3097).

#### New Contributors

- [@&#8203;MartinMoizard](https://github.com/MartinMoizard) made their
first contribution in
[pointfreeco/swift-composable-architecture#3078
- [@&#8203;JOyo246](https://github.com/JOyo246) made their first
contribution in
[pointfreeco/swift-composable-architecture#3092
- [@&#8203;pwszebor](https://github.com/pwszebor) made their first
contribution in
[pointfreeco/swift-composable-architecture#3098
- [@&#8203;Alex293](https://github.com/Alex293) made their first
contribution in
[pointfreeco/swift-composable-architecture#3097
- [@&#8203;gibachan](https://github.com/gibachan) made their first
contribution in
[pointfreeco/swift-composable-architecture#3099
- [@&#8203;leeari95](https://github.com/leeari95) made their first
contribution in
[pointfreeco/swift-composable-architecture#3110
- [@&#8203;btr-better](https://github.com/btr-better) made their first
contribution in
[pointfreeco/swift-composable-architecture#3107
- [@&#8203;Matt54](https://github.com/Matt54) made their first
contribution in
[pointfreeco/swift-composable-architecture#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://github.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://github.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDkuNCIsInVwZGF0ZWRJblZlciI6IjM2LjEwOS40IiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: Self-hosted Renovate Bot <361546+cgrindel-self-hosted-renovate[bot]@users.noreply.github.enterprise.com>
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