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

Swift Package support #20

Merged
merged 18 commits into from
Jun 9, 2023

Conversation

DivineDominion
Copy link
Contributor

@DivineDominion DivineDominion commented Jun 8, 2023

Going through WordCounter dependencies, I figured that this lib might nicely work as a Swift Package as well :)

Caveat:

Ideally, your tagged releases would drop the v prefix, so v2.0.0 becomes 2.0.0 for SPM to work.

File tree

To satisfy SPM, Cocoapods, and Xcode, I went with these directories (see comments inline):

$ tree
.
├── DateRangePicker.podspec
├── DateRangePicker.xcodeproj
│   ├── project.pbxproj
│   ├── project.xcworkspace
│   │   ├── contents.xcworkspacedata
│   │   ├── xcshareddata
│   │   │   ├── IDEWorkspaceChecks.plist
│   │   │   └── swiftpm
│   │   │       └── configuration
│   │   └── xcuserdata
│   │       └── ctm.xcuserdatad
│   │           └── UserInterfaceState.xcuserstate
│   ├── xcshareddata
│   │   └── xcschemes
│   │       └── DateRangePicker.xcscheme
│   └── xcuserdata
│       └── ctm.xcuserdatad
│           └── xcschemes
│               └── xcschememanagement.plist
├── DateRangePickerDemo
│   ├── AppDelegate.swift
│   ├── Assets.xcassets
│   │   └── AppIcon.appiconset
│   │       └── Contents.json
│   ├── Base.lproj
│   │   └── Main.storyboard
│   ├── Info.plist
│   ├── ViewController.swift
│   ├── de.lproj
│   │   └── Main.strings
│   ├── ru.lproj
│   │   └── Main.strings
│   └── zh-Hans.lproj
│       └── Main.strings
├── LICENSE
├── Package.swift
├── README.md
├── Screenshots
│   ├── ControlVariants.png
│   ├── Menu.png
│   └── Popover.png
├── Sources
│   └── DateRangePicker
│       ├── DateErrorLogger.swift
│       ├── DateRange.swift
│       ├── DateRangeButton.swift
│       ├── DateRangePickerView.swift
│       ├── DoubleClickDateRangePicker.swift
│       ├── ExpandedDateRangePickerController.swift
│       ├── LocalizationHelper.swift
│       ├── NSDate_DateRangePicker.swift
│       └── Resources      // ℹ️ This is the SPM convention to bundle assets and .strings
│           ├── Base.lproj
│           │   └── ExpandedDateRangePickerController.xib
│           ├── DateRangePicker_Colors.xcassets
│           │   ├── Contents.json
│           │   └── DateRangePicker_separator.colorset
│           │       └── Contents.json
│           ├── de.lproj
│           │   ├── ExpandedDateRangePickerController.strings
│           │   └── Localizable.strings
│           ├── fr.lproj
│           │   ├── ExpandedDateRangePickerController.strings
│           │   └── Localizable.strings
│           ├── ru.lproj
│           │   ├── ExpandedDateRangePickerController.strings
│           │   └── Localizable.strings
│           └── zh-Hans.lproj
│               ├── ExpandedDateRangePickerController.strings
│               └── Localizable.strings
├── Tests
│   └── DateRangePickerTests
│       ├── AssetTests.swift
│       ├── DateRangeTest.swift
│       └── NSDate_DateRangePickerTest.swift
├── Xcode               // ℹ️ Came up with this to bundle the Xcode framework stuff 🤷 WDYT?
│   ├── DateRangePicker
│   │   ├── DateRangePicker.h
│   │   └── Info.plist
│   └── DateRangePickerTests
│       └── Info.plist
└── localize.sh

This change is Reviewable

@@ -0,0 +1,23 @@
// swift-tools-version: 5.6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could lower this but I don't know how far down makes sense in practice, given that Xcode 14 is more or less mandatory since Ventura


open class ExpandedDateRangePickerController: NSViewController {
/// - Note: Testing seam to verify the color is loadable from all build products.
@available(macOS 10.14, *)
static var separatorColor: NSColor? {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was actually the hardest part. I learned that swift build works fine, but swift test (trying to access this color) revealed that the asset cannot be loaded outside of Xcode. This somewhat makes sense, given the .xcassets file name :) (I removed the resources: setting from the Package manifest afterwards because it has no effect in the 1 case where it produces a warning, i.e. the command line)

As the comments explain, the problematic code path is irrelevant because this library requires Xcode anyway. Command line builds of AppKit packages don't make sense because Swift packages don't support these yet, anyway, AFAIK.

If you want to see the problem of command line usage in isolation, this project demonstrates the issue:
https://github.com/marcomasser/ColorAssetsInPackageManager

@DivineDominion DivineDominion removed the request for review from MrMage June 8, 2023 08:18
@DivineDominion DivineDominion changed the title Swift Package support WIP Swift Package support Jun 8, 2023
@@ -50,7 +50,7 @@
<binding destination="-2" name="maxValue" keyPath="maxDate" id="z8H-Ip-gnA"/>
</connections>
</datePicker>
<datePicker verticalHuggingPriority="750" translatesAutoresizingMaskIntoConstraints="NO" id="HSY-I2-CQn" customClass="DoubleClickDateRangePicker" customModule="DateRangePicker" customModuleProvider="target">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curiously, module inference needs to be disabled, it seems, when Nibs are loaded from Packages:
2023-06-08 10-22-32 Xcode - Analytics — ExpandedDateRangePickerController xib@2x

@DivineDominion DivineDominion changed the title WIP Swift Package support Swift Package support Jun 8, 2023
Copy link
Collaborator

@MrMage MrMage 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 the record, did you check whether embedding this as a CocoaPod still works?

Also, once this is merged, please ping me to tag a new release without the "v" :-)

ℹ️ Came up with this to bundle the Xcode framework stuff 🤷 WDYT?

Sorry, what do you mean with that?

Reviewed 27 of 31 files at r1, 3 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @DivineDominion)


DateRangePicker.podspec line 4 at r3 (raw file):

    s.name                  = 'DateRangePicker'
    s.version               = '5.0'
    # TODO: Update URLs to Timing-GmbH/DateRangePicker

Please feel free to do this here as well, also elsewhere :-)


Package.swift line 1 at r1 (raw file):

Previously, DivineDominion (Christian Tietze) wrote…

We could lower this but I don't know how far down makes sense in practice, given that Xcode 14 is more or less mandatory since Ventura

5.6 should be more than fine 👍


Package.swift line 9 at r3 (raw file):

	defaultLocalization: "en",
	platforms: [
		.macOS(.v10_13)

nit: trailing comma


Sources/DateRangePicker/ExpandedDateRangePickerController.swift line 78 at r1 (raw file):

Previously, DivineDominion (Christian Tietze) wrote…

This was actually the hardest part. I learned that swift build works fine, but swift test (trying to access this color) revealed that the asset cannot be loaded outside of Xcode. This somewhat makes sense, given the .xcassets file name :) (I removed the resources: setting from the Package manifest afterwards because it has no effect in the 1 case where it produces a warning, i.e. the command line)

As the comments explain, the problematic code path is irrelevant because this library requires Xcode anyway. Command line builds of AppKit packages don't make sense because Swift packages don't support these yet, anyway, AFAIK.

If you want to see the problem of command line usage in isolation, this project demonstrates the issue:
https://github.com/marcomasser/ColorAssetsInPackageManager

Thanks for the elaboration! As an alternative, could we just require macOS 10.15+, or do you need to support older versions of macOS?


Sources/DateRangePicker/ExpandedDateRangePickerController.swift line 85 at r3 (raw file):

					return NSColor(genericGamma22White: 1, alpha: 0.098)
				} else {
					return NSColor(genericGamma22White: 1, alpha: 0.098)

Suggestion:

return NSColor(genericGamma22White: 0, alpha: 0.098)

Sources/DateRangePicker/LocalizationHelper.swift line 13 at r3 (raw file):

func getBundle() -> Bundle {
	#if SWIFT_PACKAGE
	return Bundle.module

Would this work in the non-SPM version as well?

Copy link
Contributor Author

@DivineDominion DivineDominion left a comment

Choose a reason for hiding this comment

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

ℹ️ Came up with this to bundle the Xcode framework stuff 🤷 WDYT?

Sorry, what do you mean with that?

I am not aware of any convention where the Xcode project files should go if you develop a Swift Package. So I grouped everything ("the Xcode framework stuff") in Xcode/.

Thanks! For the record, did you check whether embedding this as a CocoaPod still works?

Works (tested as a local pod) 👍

Reviewable status: 28 of 31 files reviewed, 3 unresolved discussions (waiting on @MrMage)


DateRangePicker.podspec line 4 at r3 (raw file):

Previously, MrMage (Daniel Alm) wrote…

Please feel free to do this here as well, also elsewhere :-)

Done.


Package.swift line 9 at r3 (raw file):

Previously, MrMage (Daniel Alm) wrote…

nit: trailing comma

Done


Sources/DateRangePicker/ExpandedDateRangePickerController.swift line 78 at r1 (raw file):

Previously, MrMage (Daniel Alm) wrote…

Thanks for the elaboration! As an alternative, could we just require macOS 10.15+, or do you need to support older versions of macOS?

I'm personally still on macOS 10.13+ 😬

Can maintain a fork, though.


Sources/DateRangePicker/ExpandedDateRangePickerController.swift line 85 at r3 (raw file):

					return NSColor(genericGamma22White: 1, alpha: 0.098)
				} else {
					return NSColor(genericGamma22White: 1, alpha: 0.098)

Done.


Sources/DateRangePicker/LocalizationHelper.swift line 13 at r3 (raw file):

Previously, MrMage (Daniel Alm) wrote…

Would this work in the non-SPM version as well?

Sorry, can you elaborate what you are asking?

Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

I am not aware of any convention where the Xcode project files should go if you develop a Swift Package. So I grouped everything ("the Xcode framework stuff") in Xcode/.

Hmm, "Xcode" feels a bit too generic, but I don't have a better idea, either. So let's keep it as-is, I guess.

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DivineDominion)


Sources/DateRangePicker/ExpandedDateRangePickerController.swift line 78 at r1 (raw file):

Previously, DivineDominion (Christian Tietze) wrote…

I'm personally still on macOS 10.13+ 😬

Can maintain a fork, though.

No problem, let's stick with 10.13, then.


Sources/DateRangePicker/LocalizationHelper.swift line 13 at r3 (raw file):

Previously, DivineDominion (Christian Tietze) wrote…

Sorry, can you elaborate what you are asking?

Would Bundle.module work in non-SPM builds as well?

Copy link
Contributor Author

@DivineDominion DivineDominion left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MrMage)


Sources/DateRangePicker/LocalizationHelper.swift line 13 at r3 (raw file):

Previously, MrMage (Daniel Alm) wrote…

Would Bundle.module work in non-SPM builds as well?

Ah, it's not known to the compiler then. So: no.

Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @DivineDominion)


Sources/DateRangePicker/LocalizationHelper.swift line 13 at r3 (raw file):

Previously, DivineDominion (Christian Tietze) wrote…

Ah, it's not known to the compiler then. So: no.

Alright, thanks!

@DivineDominion DivineDominion merged commit 6b87a66 into Timing-GmbH:master Jun 9, 2023
@DivineDominion DivineDominion deleted the ctietze/swiftpm branch June 9, 2023 13:08
@DivineDominion
Copy link
Contributor Author

Also, once this is merged, please ping me to tag a new release without the "v" :-)

@MrMage Ping 🔔

Looking forward to using the new version 🎉

@MrMage
Copy link
Collaborator

MrMage commented Jun 9, 2023

Released!

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.

2 participants