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 Swift Package Manager Support #24

Closed
wants to merge 6 commits into from

Conversation

Sherlouk
Copy link
Contributor

@Sherlouk Sherlouk commented Sep 27, 2020

⚠️ Work in progress...

Based on conversation in #1
This is also extending from the work in #21 enabling SnapshotTesting support (which is the default for Swift Package Manager installation).

Changes:

  • Restructured repository so source code more easily followed the Swift Package format. This includes pulling public headers out into an include directory which simplifies setup.
  • Updated Podspec to use the new structure ensuring.
  • Moved UnitTests to the package, rather than in the example project.
  • Duplicated the entire SnapshotTests scheme, replacing the Uber syntax with Pointfree's syntax. I was tempted to try and have the SnapshotTesting subspec use the same suite of tests to ensure both installation methods have the exact same output, but didn't run with this yet.
  • Increased deployment target to iOS 11

Notes:

  • The example project doesn't like having both the CocoaPods version and the Swift Package Manager version building at the same time. As it stands, you need to enabling one or the other via the scheme but both do work when run individually.
  • I have not included the snapshots for the Swift Package Manager target, though I have verified everything seems to be working as expected.
  • This is currently using a forked version of fishhook for Swift Package Manager only. The CocoaPods installation is completely unaffected. If we continue with this then it'd make sense to open a PR for my version of fishhook into the parent repository -- but we could easily and seamlessly change this dependency at a later date without really any bother to our users.

TODO:

  • Find workaround for the CocoaPods+SPM issue noted above.
  • Figure out how to implement the SnapshotTestCase for SnapshotTesting. SnapshotTesting doesn't (yet, there have been many discussions...) support adding simulator details such as OS, screen size or screen scale to the image name like Uber's library does.
  • CI Fixed
  • Documentation Changes

@NickEntin - would love to hear your opinions on the work here and particularly if you have any thoughts on how to approach the two top TODOs and honestly listing all the issues with the work here that you'd want changing! Thanks 🙌

@NickEntin
Copy link
Collaborator

This is looking great so far!

The example project doesn't like having both the CocoaPods version and the Swift Package Manager version building at the same time. As it stands, you need to enabling one or the other via the scheme but both do work when run individually.

That's fine. We can have two schemes - one for each of the dependency managers, something like AccessibilitySnapshotDemo (FBSnapshotTestCase) and AccessibilitySnapshotDemo (SnapshotTesting). We don't need to test both using the same scheme.

Figure out how to implement the SnapshotTestCase for SnapshotTesting. SnapshotTesting doesn't (yet, there have been many discussions...) support adding simulator details such as OS, screen size or screen scale to the image name like Uber's library does.

Hmm, I don't have enough experience with SnapshotTesting to know what all options are available here. @efirestone do you have any ideas how we can do this?

@Sherlouk
Copy link
Contributor Author

Okay so both schemes are happily working locally on Xcode 12 but CI is having a cry because.. well.. it's not on Xcode 12. Also got CocoaPods lint all happy 👍

We might need to make an entirely separate project to get the Swift Package version working on CI. Thoughts?

Will wait on guidance around how we want to handle the different scales as well.

@NickEntin
Copy link
Collaborator

Should be able to rebase on master now to avoid the fishhook dependency

@Sherlouk
Copy link
Contributor Author

Awesome! I'll take a fresh branch and go at it again this week (hopefully 🤞) - I'll start a new draft PR and I'll leave out most of the changes to tests (for now) focussing on the restructure and basic SPM support 👍

@Sherlouk Sherlouk mentioned this pull request Nov 13, 2020
1 task
@Sherlouk Sherlouk closed this Nov 18, 2020
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