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

Update the messaging around the use of deinit for teardown #551

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

medreisbach
Copy link
Contributor

@medreisbach medreisbach commented Jul 18, 2024

resolves rdar://132007317

Checklist:

  • Code and documentation should follow the style of the Style Guide.
  • If public symbols are renamed or modified, DocC references should be updated.

@medreisbach medreisbach added the documentation Improvements or additions to documentation label Jul 18, 2024
@medreisbach medreisbach self-assigned this Jul 18, 2024
Copy link
Contributor

@grynspan grynspan left a comment

Choose a reason for hiding this comment

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

Minor quibbles as noted.

Copy link
Contributor

@stmontgomery stmontgomery left a comment

Choose a reason for hiding this comment

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

Thanks for improving these docs!

In XCTest, you can also schedule work to run after a test using the [`tearDown()`](https://developer.apple.com/documentation/xctest/xctest/3856482-teardown)
family of functions. When writing tests using the testing library, adopt structured
[concurrency](https://docs.swift.org/swift-book/documentation/the-swift-programming-language/concurrency/)
and take advantage of Swift to handle cleanup for you where possible rather
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify what feature(s) of Swift concurrency, or which APIs, you mean here which assist with cleanup? I can think of defer, but I would consider that a basic language feature rather than a concurrency feature.

Copy link
Member

Choose a reason for hiding this comment

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

I think what we should aim to push people towards is using with style methods e.g. try await withNetworkConnection { connection in ... }. This is the pattern that recommend to people to have guaranteed lifetimes with structured concurrency.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's something they have to write themselves, and explaining how to write one would get us into the weeds a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is precedent for describing how testing helps you design nicer APIs and vice versa, and it's something Apple has already published in the Xcode documentation: https://developer.apple.com/documentation/xcode/updating-your-existing-codebase-to-accommodate-unit-tests

So maybe we could take a similar approach here, where we show a with… function as an example of designing API+test together, but (as a separate task) create another doc that shows the thinking behind that design?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be an appropriate place to use a snippet?

Copy link
Member

Choose a reason for hiding this comment

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

I think an example here would be extremely helpful for users trying to migrate

Use code voice around Swift keywords.
Remove final as a requirement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants