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

Add macOS and watchOS support to Package.swift #1

Closed
wants to merge 5 commits into from

Conversation

mackoj
Copy link

@mackoj mackoj commented Jun 28, 2023

Hi,

Thank you for creating this library, upon which I rely daily, along with my library EnvironmentVariationPreview, to enhance the testing coverage of my work.

To enable the usage of swift package diagnose-api-breaking-changes, I kindly request adding support for macOS in the Package.swift file. Additionally, if feasible, it would be great to include support for watchOS as well.

Thank you once again!

What does this PR modify:

  • Add missing import for UIKit.
  • Add platforms availability marker.

Now, swift package diagnose-api-breaking-changes main seems to build the branch version, but it can't build the main branch for macOS since it is not compatible or buildable.

This PR don't change any logic of swiftui-preview-snapshots it just add a few @available marker.

Best regards,

@mackoj mackoj changed the title Add support for macOS and watchOS in Package.swift Add macOS and watchOS support to Package.swift Jun 28, 2023
@mackoj mackoj marked this pull request as draft June 28, 2023 17:09
@mackoj
Copy link
Author

mackoj commented Jun 29, 2023

Hi @jflan-dd,

What do you think about this PR ?

@mackoj mackoj marked this pull request as ready for review June 30, 2023 08:18
@@ -11,9 +11,11 @@
// or implied. See the License for the specific language governing permissions and limitations under
// the License.

#if canImport(UIKit)
Copy link

Choose a reason for hiding this comment

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

I really hope watchOS support gets approved. Should this if import just wrap the UIKit import and why is this needed?

Copy link
Author

Choose a reason for hiding this comment

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

Because UIImage is used and UIKit import is missing.

@jflan-dd
Copy link
Contributor

Hey @mackoj!

Thanks for the contribution!

Sorry I missed the notification on this PR. I'll take a look at it today.

@jflan-dd
Copy link
Contributor

The swift-snapshot-testing library that this library is built on top of doesn't currently support watchOS in their package manifest. If there's useful snapshotting that can be done on watchOS during tests we could try and get supported added to their library.
https://github.com/pointfreeco/swift-snapshot-testing/blob/dc46eeb3928a75390651fac6c1ef7f93ad59a73b/Package.swift#L4-L10

@mackoj
Copy link
Author

mackoj commented Aug 15, 2023

So let's remove watchOS for the moment I'll update the PR tomorrow.

@jflan-dd
Copy link
Contributor

@mackoj I think putting all of the assertSnapshots methods inside of a #if canImport(UIKit) is going to make the library tough to use on macOS.

Maybe we should update the various assertSnapshots methods to be generic over the format snapshotting format, and then provide default implementations when UIKit is available.

    public func assertSnapshots<Format>(
        as snapshotting: Snapshotting<AnyView, Format>,
        record recording: Bool = false,
        file: StaticString = #file,
        testName: String = #function,
        line: UInt = #line
    ) {
        // Unchanged...
    }

and

#if canImport(UIKit)
import UIKit

extension PreviewSnapshots {
    /// Doc comment
    public func assertSnapshots(
        record recording: Bool = false,
        file: StaticString = #file,
        testName: String = #function,
        line: UInt = #line
    ) {
        assertSnapshots(as: .image, record: recording, file: file, testName: testName, line: line)
    }
}

// MARK: - PreviewSnapshots.assertSnapshots + modify

extension PreviewSnapshots {
    /// Doc comment
    public func assertSnapshots<Modified: View>(
        record recording: Bool = false,
        file: StaticString = #file,
        testName: String = #function,
        line: UInt = #line,
        modify: (AnyView) -> Modified
    ) {
        assertSnapshots(as: .image, record: recording, file: file, testName: testName, line: line, modify: modify)
    }
}
#endif

@mackoj
Copy link
Author

mackoj commented Aug 16, 2023

This is a better approach for sure my goal was merely to make it compile on macOS but having it working macOS is even better ? do you want to do it ? should I close my PR?

@mackoj
Copy link
Author

mackoj commented Aug 16, 2023

#3 Is better than this PR so I close it

@mackoj mackoj closed this Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants