Skip to content

Add watchOS as a supported destination #1438

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.

Sign up for GitHub

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

Jump to bottom
Merged
yonaskolb merged 4 commits into from
Feb 13, 2024
Merged

Add watchOS as a supported destination #1438

yonaskolb merged 4 commits into from
Feb 13, 2024

Conversation

@tatsuky
Copy link
Contributor

@tatsuky tatsuky commented Jan 23, 2024 •
edited
Loading

Hi, I'm new to this project. This PR adds the watchOS support for the supportedDestinations configuration, initially added in #1336.

Changes

  • Added the watchOS case to the SupportedDestination enum.
  • Added a configuration template for watchOS.
  • Added test cases.

Tests

You can use the following example spec to verify the changes.

name: MyProject
options:
  bundleIdPrefix: com.myapp
targets:
  MyWatchKitApp:
    type: application
    platform: watchOS
    sources: [MyWatchKitApp]
    settings:
      MARKETING_VERSION: 1
      CURRENT_PROJECT_VERSION: 1
    dependencies:
      - target: MyFramework
    info:
      path: MyWatchKitApp/Info.plist
      properties:
        WKCompanionAppBundleIdentifier: com.myapp
        WKApplication: true
  MyFramework:
    type: framework
    supportedDestinations: [iOS, watchOS]
    platform: iOS
    sources: [MyFramework]
    settings:
      GENERATE_INFOPLIST_FILE: YES

Thanks.

tatsuky
tatsuky commented Jan 23, 2024
View reviewed changes
Sources/ProjectSpec/SupportedDestination.swift Outdated
Comment on lines 46 to 47
case .watchOS:
return 5
Copy link
Contributor Author

@tatsuky tatsuky Jan 23, 2024

Choose a reason for hiding this comment

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

Hi @yonaskolb and @amatig,
I'm wondering why the priorities are currently in this order and where watchOS should be. Any thoughts?

Copy link
Contributor Author

@tatsuky tatsuky Jan 23, 2024

Choose a reason for hiding this comment

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

Adding test cases helped me realize this - TARGETED_DEVICE_FAMILY and SUPPORTED_PLATFORMS are ordered based on this priority. watchOS is 4, meaning it needs to be between tvOS (3) and visionOS (7).

giginet
giginet approved these changes Jan 23, 2024
View reviewed changes
Copy link
Collaborator

@giginet giginet left a comment

Choose a reason for hiding this comment

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

Great contribution! I wished this feature 💯

I'll test this feature for my use case.

Tests/XcodeGenKitTests/ProjectGeneratorTests.swift
}


$0.it("supportedDestinations merges settings - iOS, watchOS") {
Copy link
Collaborator

@giginet giginet Jan 23, 2024

Choose a reason for hiding this comment

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

These tests can be parameterized tests to reduce redundancy.

But it's better existing test cases also become parameterized tests.
So I think it's good at this time.

@giginet giginet mentioned this pull request Jan 29, 2024
Closed
yonaskolb
yonaskolb approved these changes Feb 13, 2024
View reviewed changes
Copy link
Owner

@yonaskolb yonaskolb left a comment

Choose a reason for hiding this comment

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

Thank you @tatsuky

@yonaskolb yonaskolb merged commit 6bbf2c6 into yonaskolb:master Feb 13, 2024
@bcardarella bcardarella mentioned this pull request Apr 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@giginet giginet giginet approved these changes

@yonaskolb yonaskolb yonaskolb approved these changes

+1 more reviewer

@PierreJanineh PierreJanineh PierreJanineh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@giginet giginet

@freddi-kit freddi-kit

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tatsuky @giginet @yonaskolb @PierreJanineh @freddi-kit