-
Notifications
You must be signed in to change notification settings - Fork 452
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
Split SwiftProtobuf into SwiftProtobufCore & SwiftProtobufFoundationCompat #1331
Conversation
ecfc965
to
56e4e5f
Compare
@testable import SwiftProtobuf | ||
@testable import SwiftProtobufCore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think none of these should change for now. If we do this move without breaking anything all the tests compile without any real change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue here is that in these tests are using internal methods (hence the @testable
). However, the re-export I'm doing in SwiftProtobuf/Exports.swift
using @_exported import SwiftProtobufCore
doesn't expose those methods, and there's no way to do it (AFAIK) without explicitly doing @testable import SwiftProtobufCore
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FranzBusch is there where @_spi()
usage comes up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might work here. Never tried to combination of @_exported import
and @_spi
together. We should definitely take a look since @_spi
has a few more upsides to @testable
as well like being able to compile in release mode without extra flags. However, I would do this investigation in a follow up since I am not sure which Swift version introduced @_spi
and if we can actually use it here.
2f7a05b
to
51ca597
Compare
Sources/SwiftProtobufFoundationCompat/SwiftProtobufContiguousBytes.swift
Outdated
Show resolved
Hide resolved
- Move SwiftProtobufContiguousBytes to SwiftProtobufCore (it was mistakenly added to SwiftProtobufFoundationCompat earlier) - Added a placeholder for extending Data with SwiftProtobufContiguousBytes conformance in SwiftProtobufFoundationCompat
ce8e0bb
to
e60ba92
Compare
To keep PR more focused. Will re-add them in a separate PR.
I think I'm good with things at this as a starting point. @tbkka @FranzBusch (and anyone else) - thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. It opens the door for us to segregate out Data usage and also sets an example that may be useful for segregating JSON and TextFormat support (which has been proposed a couple of times but no one has done the experiment to see whether it's worth doing).
Unless someone does it before me, I'll do a squash+merge tomorrow; just so anyone else that wants to comment gets a chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and happy to get this merged!
Motivation
This is the first step of several in the path to removing the usage of
Data
(and eventuallyFoundation
) fromswift-protobuf
. This is related to #816.Modifications
SwiftProtobuf
module toSwiftProtobufCore
SwiftProtobufFoundationCompat
module, which is currently empty.SwiftProtobuf
module that only re-exports bothSwiftProtobufFoundationCore
andSwiftProtobufFoundationCompat
.Makefile
and regenerated necessary files.Result
We now have different modules that lay the groundwork for the migration proposed in #816. Other related, currently-open PRs will be updated once this one is landed.