-
Notifications
You must be signed in to change notification settings - Fork 69
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 support for installing via Swift Package Manager #1
Comments
In order to add SPM support for AccessibilitySnapshot, we need its dependencies to work with SPM first. There's an open issue for iOSSnapshotTestCase to add support (uber/ios-snapshot-test-case#91) and adding it for fishhook should be relatively straightforward. |
Any progress on this Nick? I understand there's a minor dependency on Uber (though a MR is open) but I'm unsure how you wanted to proceed with fishhook -- would be nice to get Core compatible with SPM at least! |
Hey James, no progress on this yet. As you said, fishhook is the only dependency needed to add SPM support to the core framework. I filed facebook/fishhook#77 to make sure it's being tracked for that project. |
🙌 Merge Request is open at facebook/fishhook#78 We've got a few projects which are SPM only so looking forward to being able to support this for SnapshotTesting |
I'm not sure if the fishhook repo is being actively monitored/managed etc by the teams at Facebook - how against the idea of using the fork which has support for it are you? Would be awesome to unblock the SPM support here especially as teams (including mine!) are transitioning to SPM! Thanks in advance 🙌 |
I commented on the fishhook PR and tagged a couple of reviewers from the most recent merged PRs to see if they can take a look. I'd love to see SPM support here unblocked soon. 🙌 My main concern with switching over to a fork is that (presumably) it would be published as a separate pod, which could potentially be problematic if consumers of AccessibilitySnapshot already use fishhook for something else (either directly or as a transitive dependency). I think @dfed mentioned a while back that there are some workarounds we could do in SPM to deal with a missing dependency, but let's give fishhook another push first and see if we can get it supported properly. |
Hey @NickEntin If you get 15 minutes I don't support you'd mind taking a gander at my very WIP branch for Swift Package Manager support? https://github.com/Sherlouk/AccessibilitySnapshot/tree/sherlouk/spm The branch is unorganised so don't worry too much about how the pods stuff is setup (Should be no problem keeping them both working at the same time, even without changing the dependencies for pod consumers). I'm hitting a problem with the visibility of one of the functions from fishhook - specifically the If you open the Package.swift and run tests for the
If we can get past that I can clean everything up and will be much closer to something that's sharable! Would appreciate any support/guidance with this 🙏 Thanks |
🤔 I'm getting the same error you're seeing, but I'm not sure exactly why it's failing. My best guess is that it has to do with the |
You were 100% right Nick! I had noticed the I patched it on my own fork and that seems to be working... I'll tidy up my fork of this repo to get everything looking beautiful and then I'll open a merge request against fishhook building on top of the existing pull request (as that on it's own will not work, as we've found out...) |
Awesome, sounds good! Sometimes it helps to not really know much about how the system works (I haven't used SPM that much so far) and have to look up how it works as you go. 😄 |
The Swift Package Manager merge request is getting pretty packed and I'm concerned it could become quite the chore to review/rebase etc. How do you feel about breaking this down into a few separate PRs and merge them one at a time?
Just feels a bit more manageable for both you (and co) and I? Thoughts? |
Yep, agreed breaking this up will make it much more manageable. I think there's still some open questions around exactly how part 2 should look, but let's go ahead and knock out part 1 first. 👍 |
Yah the second part is an interesting one. I had considered making it so we could compiler flag the actual snapshotting so the same test files could be used for all the test runs but decided against it as the tests are also documentation for a library like this so you want them to be as super simple and clear as possible! In theory it is completely optional too - was more just wanting a method to test that the SPM version of the lib is also working as expected. Could maybe just do one test class for that though and leave the others as is? |
Resolved by #47 |
No description provided.
The text was updated successfully, but these errors were encountered: