[Traits] Disallow disabling default traits of a package without traits #8326
Conversation
FranzBusch
requested review from
bnbarham,
MaxDesiatov,
jakepetroules,
xedin,
dschaefer2,
shawnhyam,
bripeticca,
plemarquand,
owenv and
bkhouri
as code owners
|
@swift-ci please test |
There was a problem hiding this comment.
Drive-by from to add a big +1 and thank you for this patch. This will make traits even more of a useful addition to the package ecosystem and unlock the ability to retrofit good API segregation for important packages in the ecosystem that would like to be more modular, but predate traits. ❤️
| @@ -179,6 +184,10 @@ extension ModuleError: CustomStringConvertible { | |||
| return """ | |||
| Trait '"\(trait)"' is not declared by package '\(package)'. | |||
| """ | |||
| case .disablingDefaultTraitsOnEmptyTraits(let parentPackage, let packageName): | |||
| return """ | |||
| Disabled default traits by package '\(parentPackage)' on package '\(packageName)' that declares no traits. This is prohibited to allow packages to adopt traits initially without causing. | |||
There was a problem hiding this comment.
This wording on this error message sounds incomplete?
There was a problem hiding this comment.
Looks good to me, just curious about about the error message for disablingDefaultTraitsOnEmptyTraits - like @simonjbeaumont mentioned it seems like it's incomplete
FranzBusch
force-pushed
the
fb-disable-default-traits-on-empty-traits
branch
from
a92d02d to
5e9eca4
Compare
|
@swift-ci please test |
FranzBusch
force-pushed
the
fb-disable-default-traits-on-empty-traits
branch
from
5e9eca4 to
fb661c0
Compare
|
@swift-ci please test |
|
@swift-ci test windows |
# Motivation Traits are a great way for package authors to offer customization of the functionality they provide. However, moving existing API behind a trait is considered an API breaking change since packages that depend on them might have disabled all default traits. This makes it almost impossible for existing packages to adopt traits for existing code. # Modifications This PR disallows disabling the default traits for packages with no traits at all. This allows package authors to move existing API behind traits once since no consumer can disable the default traits before. # Result With this change we create a migration path for existing packages to traits without them breaking their APIs.
FranzBusch
force-pushed
the
fb-disable-default-traits-on-empty-traits
branch
from
fb661c0 to
90bf5ca
Compare
|
@swift-ci please test |
|
@swift-ci please test Windows |
|
@swift-ci please test windows |
Motivation
Traits are a great way for package authors to offer customization of the functionality they provide. However, moving existing API behind a trait is considered an API breaking change since packages that depend on them might have disabled all default traits. This makes it almost impossible for existing packages to adopt traits for existing code.
Modifications
This PR disallows disabling the default traits for packages with no traits at all. This allows package authors to move existing API behind traits once since no consumer can disable the default traits before.
Result
With this change we create a migration path for existing packages to traits without them breaking their APIs.