[Traits] Replace isDefault with default trait
#7703
Conversation
FranzBusch
requested review from
bnbarham,
MaxDesiatov,
jakepetroules,
francescomikulis and
xedin
as code owners
FranzBusch
changed the title
isDefault with defaults traitisDefault with default trait
MaxDesiatov
added
pending evolution proposal
enhancement
modules graph
| @@ -107,11 +107,6 @@ public struct ManifestValidator { | |||
| continue | |||
| } | |||
|
|
|||
| guard traitName.lowercased() != "default" && traitName.lowercased() != "defaults" else { | |||
| diagnostics.append(.defaultTraitName()) | |||
There was a problem hiding this comment.
Wouldn't we want to keep this error? A user could still declare both .default and a trait named "defaults" at the same time with unintended consequences, right?
There was a problem hiding this comment.
I intentionally removed it because a user declaring a default trait is just an alternative spelling to using .default(enabledTraits:). I don't want to ban this since a user could add a different description if they wanted. With this change the default trait is just serialised as any other trait.
I also thought about keeping the ban on declaring a defaults but decided that it is not worth the restriction since it poses no harm. I could add the ban for defaults back and just remove the default one but I don't fell strongly about it
There was a problem hiding this comment.
The spelling needs to be adjusted then. The helper function is called .default, while the trait is called "defaults". How's about keeping the function .default as it is now, but making the string spelling more explicit: "defaultTraits"?
There was a problem hiding this comment.
Ah good point I gotta change the backing String. I do prefer default though. The traits are implied since this is part of a set of traits in the end and this would feel redundant to me.
FranzBusch
force-pushed
the
fb-defaults-trait
branch
from
485f7d3 to
6f020d3
Compare
|
@swift-ci please test |
FranzBusch
force-pushed
the
fb-defaults-trait
branch
from
6f020d3 to
6283b15
Compare
|
@swift-ci please test |
|
@swift-ci test windows |
FranzBusch
force-pushed
the
fb-defaults-trait
branch
from
6283b15 to
e626518
Compare
|
@swift-ci please test |
|
@swift-ci please test |
FranzBusch
force-pushed
the
fb-defaults-trait
branch
from
e626518 to
99f1844
Compare
|
@swift-ci please test |
# Motivation The current spelling uses `isDefault` per trait to declare the default traits. This makes it hard to see which traits are actually the default traits. # Modification Instead of using an explicit `isDefault` per trait this PR now uses a `.defaults(enabledTraits:)` overload which just uses the `"default"` trait under the hood. This simplifies the logic tremendously to handle default traits. # Result Nicer spelling for default traits and easier implementation.
FranzBusch
force-pushed
the
fb-defaults-trait
branch
from
99f1844 to
3865f78
Compare
|
@swift-ci please test |
|
@swift-ci test windows |
1 similar comment
|
@swift-ci test windows |
Motivation
The current spelling uses
isDefaultper trait to declare the default traits. This makes it hard to see which traits are actually the default traits.Modification
Instead of using an explicit
isDefaultper trait this PR now uses a.defaults(enabledTraits:)overload which just uses the"default"trait under the hood. This simplifies the logic tremendously to handle default traits.Result
Nicer spelling for default traits and easier implementation.