Skip to content

[Traits] Replace isDefault with default trait #7703

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 25, 2024
Merged

[Traits] Replace isDefault with default trait #7703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions Fixtures/Traits/Example/Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,24 @@
let package = Package(
name: "TraitsExample",
traits: [
Trait(name: "Package1", isDefault: true),
Trait(name: "Package2", isDefault: true),
Trait(name: "Package3", isDefault: true),
Trait(name: "Package4", isDefault: true),
.default(
enabledTraits: [
"Package1",
"Package2",
"Package3",
"Package4",
"BuildCondition1",
]
),
"Package1",
"Package2",
"Package3",
"Package4",
"Package5",
"Package7",
"Package9",
"Package10",
Trait(name: "BuildCondition1", isDefault: true),
"BuildCondition1",
"BuildCondition2",
"BuildCondition3",
],
Expand Down
7 changes: 4 additions & 3 deletions Fixtures/Traits/Package3/Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ let package = Package(
),
],
traits: [
Trait(name: "Package3Trait1", enabledTraits: ["Package3Trait2"]),
Trait(name: "Package3Trait2", enabledTraits: ["Package3Trait3"]),
Trait(name: "Package3Trait3", isDefault: true),
.default(enabledTraits: ["Package3Trait3"]),
.trait(name: "Package3Trait1", enabledTraits: ["Package3Trait2"]),
.trait(name: "Package3Trait2", enabledTraits: ["Package3Trait3"]),
"Package3Trait3",
],
targets: [
.target(
Expand Down
3 changes: 2 additions & 1 deletion Fixtures/Traits/Package4/Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ let package = Package(
),
],
traits: [
Trait(name: "Package4Trait1", isDefault: true),
.default(enabledTraits: ["Package4Trait1"]),
"Package4Trait1",
],
targets: [
.target(
Expand Down
2 changes: 1 addition & 1 deletion Sources/PackageDescription/PackageDependencyTrait.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ extension Package.Dependency {
@available(_PackageDescription, introduced: 999.0)
public struct Trait: Hashable, Sendable, ExpressibleByStringLiteral {
/// Enables all default traits of a package.
public static let defaults = Self.init(name: "defaults")
public static let defaults = Self.init(name: "default")

/// A condition that limits the application of a dependencies trait.
public struct Condition: Hashable, Sendable {
Expand Down
1 change: 0 additions & 1 deletion Sources/PackageDescription/PackageDescriptionSerialization.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,6 @@ enum Serialization {
struct Trait: Hashable, Codable {
let name: String
let description: String?
let isDefault: Bool
let enabledTraits: Set<String>
}

Expand Down
1 change: 0 additions & 1 deletion Sources/PackageDescription/PackageDescriptionSerializationConversion.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,6 @@ extension Serialization.Trait {
init(_ trait: PackageDescription.Trait) {
self.name = trait.name
self.description = trait.description
self.isDefault = trait.isDefault
self.enabledTraits = trait.enabledTraits
}
}
Expand Down
18 changes: 9 additions & 9 deletions Sources/PackageDescription/Trait.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@
@_spi(ExperimentalTraits)
@available(_PackageDescription, introduced: 999.0)
public struct Trait: Hashable, ExpressibleByStringLiteral {
/// Declares the default traits for this package.
public static func `default`(enabledTraits: Set<String>) -> Self {
.init(
name: "default",
description: "The default traits of this package.",
enabledTraits: enabledTraits
)
}

/// The trait's canonical name.
///
/// This is used when enabling the trait or when referring to it from other modifiers in the manifest.
Expand All @@ -35,9 +44,6 @@ public struct Trait: Hashable, ExpressibleByStringLiteral {
/// Use this to explain what functionality this trait enables.
public var description: String?

/// A boolean indicating wether the trait is enabled by default.
public var isDefault: Bool

/// A set of other traits of this package that this trait enables.
public var enabledTraits: Set<String>

Expand All @@ -46,17 +52,14 @@ public struct Trait: Hashable, ExpressibleByStringLiteral {
/// - Parameters:
/// - name: The trait's canonical name.
/// - description: The trait's description.
/// - isDefault: A boolean indicating wether the trait is enabled by default.
/// - enabledTraits: A set of other traits of this package that this trait enables.
public init(
name: String,
description: String? = nil,
isDefault: Bool = false,
enabledTraits: Set<String> = []
) {
self.name = name
self.description = description
self.isDefault = isDefault
self.enabledTraits = enabledTraits
}

Expand All @@ -69,18 +72,15 @@ public struct Trait: Hashable, ExpressibleByStringLiteral {
/// - Parameters:
/// - name: The trait's canonical name.
/// - description: The trait's description.
/// - isDefault: A boolean indicating wether the trait is enabled by default.
/// - enabledTraits: A set of other traits of this package that this trait enables.
public static func trait(
name: String,
description: String? = nil,
isDefault: Bool = false,
enabledTraits: Set<String> = []
) -> Trait {
.init(
name: name,
description: description,
isDefault: isDefault,
enabledTraits: enabledTraits
)
}
Expand Down
4 changes: 2 additions & 2 deletions Sources/PackageGraph/ModulesGraph+Loading.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,7 +829,7 @@ private func calculateEnabledTraits(
) throws -> Set<String> {
// This the point where we flatten the enabled traits and resolve the recursive traits
var recursiveEnabledTraits = explictlyEnabledTraits ?? []
let areDefaultsEnabled = recursiveEnabledTraits.remove("defaults") != nil
let areDefaultsEnabled = recursiveEnabledTraits.remove("default") != nil

// We are going to calculate which traits are actually enabled for a node here. To do this
// we have to check if default traits should be used and then flatten all the enabled traits.
Expand All @@ -843,7 +843,7 @@ private func calculateEnabledTraits(

// We have to enable all default traits if no traits are enabled or the defaults are explicitly enabled
if explictlyEnabledTraits == nil || areDefaultsEnabled {
recursiveEnabledTraits.formUnion(manifest.traits.lazy.filter { $0.isDefault }.map { $0.name })
recursiveEnabledTraits.formUnion(manifest.traits.first { $0.name == "default" }?.enabledTraits ?? [])
}

while true {
Expand Down
1 change: 0 additions & 1 deletion Sources/PackageLoading/ManifestJSONParser.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,7 +605,6 @@ extension TraitDescription {
self.init(
name: trait.name,
description: trait.description,
isDefault: trait.isDefault,
enabledTraits: trait.enabledTraits
)
}
Expand Down
9 changes: 0 additions & 9 deletions Sources/PackageLoading/ManifestLoader+Validation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,6 @@ public struct ManifestValidator {
continue
}

guard traitName.lowercased() != "default" && traitName.lowercased() != "defaults" else {
diagnostics.append(.defaultTraitName())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@MaxDesiatov MaxDesiatov Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

continue
}

for (index, unicodeScalar) in traitName.unicodeScalars.enumerated() {
let properties = unicodeScalar.properties

Expand Down Expand Up @@ -370,10 +365,6 @@ extension Basics.Diagnostic {
.error("Empty strings are not allowed as trait names")
}

static func defaultTraitName() -> Self {
.error("Traits are not allowed to be named 'default' or 'defaults' to avoid confusion with default traits")
}

static func invalidFirstCharacterInTrait(firstCharater: UnicodeScalar, trait: String) -> Self {
.error("Invalid first character (\(firstCharater)) in trait \(trait). The first character must be a Unicode XID start character (most letters), a digit, or _.")
}
Expand Down
6 changes: 0 additions & 6 deletions Sources/PackageModel/Manifest/TraitDescription.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@ public struct TraitDescription: Sendable, Hashable, Codable, ExpressibleByString
/// The trait's description.
public var description: String?

/// A boolean indicating wether the trait is enabled by default.
public var isDefault: Bool

/// A set of other traits of this package that this trait enables.
public var enabledTraits: Set<String>

Expand All @@ -30,17 +27,14 @@ public struct TraitDescription: Sendable, Hashable, Codable, ExpressibleByString
/// - Parameters:
/// - name: The trait's canonical name.
/// - description: The trait's description.
/// - isDefault: A boolean indicating wether the trait is enabled by default.
/// - enabledTraits: A set of other traits of this package that this trait enables.
public init(
name: String,
description: String? = nil,
isDefault: Bool = false,
enabledTraits: Set<String> = []
) {
self.name = name
self.description = description
self.isDefault = isDefault
self.enabledTraits = enabledTraits
}

Expand Down
8 changes: 4 additions & 4 deletions Sources/SPMTestSupport/PackageDependencyDescriptionExtensions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ package extension PackageDependency {
deprecatedName: String? = nil,
path: AbsolutePath,
productFilter: ProductFilter = .everything,
traits: Set<Trait> = [.init(name: "defaults")]
traits: Set<Trait> = [.init(name: "default")]
) -> Self {
let identity = identity ?? PackageIdentity(path: path)
return .fileSystem(
Expand All @@ -40,7 +40,7 @@ package extension PackageDependency {
path: AbsolutePath,
requirement: SourceControl.Requirement,
productFilter: ProductFilter = .everything,
traits: Set<Trait> = [.init(name: "defaults")]
traits: Set<Trait> = [.init(name: "default")]
) -> Self {
let identity = identity ?? PackageIdentity(path: path)
return .localSourceControl(
Expand All @@ -59,7 +59,7 @@ package extension PackageDependency {
url: SourceControlURL,
requirement: SourceControl.Requirement,
productFilter: ProductFilter = .everything,
traits: Set<Trait> = [.init(name: "defaults")]
traits: Set<Trait> = [.init(name: "default")]
) -> Self {
let identity = identity ?? PackageIdentity(url: url)
return .remoteSourceControl(
Expand All @@ -76,7 +76,7 @@ package extension PackageDependency {
identity: String,
requirement: Registry.Requirement,
productFilter: ProductFilter = .everything,
traits: Set<Trait> = [.init(name: "defaults")]
traits: Set<Trait> = [.init(name: "default")]
) -> Self {
return .registry(
identity: .plain(identity),
Expand Down
8 changes: 4 additions & 4 deletions Tests/FunctionalTests/TraitTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ final class TraitTests: XCTestCase {

func testTraits_whenTraitUnification() async throws {
try await fixture(name: "Traits") { fixturePath in
let (stdout, stderr) = try await executeSwiftRun(fixturePath.appending("Example"), "Example", extraArgs: ["--experimental-traits", "defaults,Package9,Package10"])
let (stdout, stderr) = try await executeSwiftRun(fixturePath.appending("Example"), "Example", extraArgs: ["--experimental-traits", "default,Package9,Package10"])
// We expect no warnings to be produced. Specifically no unused dependency warnings.
XCTAssertFalse(stderr.contains("warning:"))
XCTAssertEqual(stdout, """
Expand All @@ -59,7 +59,7 @@ final class TraitTests: XCTestCase {

func testTraits_whenTraitUnification_whenSecondTraitNotEnabled() async throws {
try await fixture(name: "Traits") { fixturePath in
let (stdout, stderr) = try await executeSwiftRun(fixturePath.appending("Example"), "Example", extraArgs: ["--experimental-traits", "defaults,Package9"])
let (stdout, stderr) = try await executeSwiftRun(fixturePath.appending("Example"), "Example", extraArgs: ["--experimental-traits", "default,Package9"])
// We expect no warnings to be produced. Specifically no unused dependency warnings.
XCTAssertFalse(stderr.contains("warning:"))
XCTAssertEqual(stdout, """
Expand All @@ -79,7 +79,7 @@ final class TraitTests: XCTestCase {

func testTraits_whenIndividualTraitsEnabled_andDefaultTraits() async throws {
try await fixture(name: "Traits") { fixturePath in
let (stdout, stderr) = try await executeSwiftRun(fixturePath.appending("Example"), "Example", extraArgs: ["--experimental-traits", "defaults,Package5,Package7,BuildCondition3"])
let (stdout, stderr) = try await executeSwiftRun(fixturePath.appending("Example"), "Example", extraArgs: ["--experimental-traits", "default,Package5,Package7,BuildCondition3"])
// We expect no warnings to be produced. Specifically no unused dependency warnings.
XCTAssertFalse(stderr.contains("warning:"))
XCTAssertEqual(stdout, """
Expand Down Expand Up @@ -186,7 +186,7 @@ final class TraitTests: XCTestCase {
let json = try JSON(bytes: ByteString(encodingAsUTF8: dumpOutput))
guard case let .dictionary(contents) = json else { XCTFail("unexpected result"); return }
guard case let .array(traits)? = contents["experimentalTraits"] else { XCTFail("unexpected result"); return }
XCTAssertEqual(traits.count, 11)
XCTAssertEqual(traits.count, 12)
}
}

Expand Down
15 changes: 9 additions & 6 deletions Tests/PackageGraphTests/ModulesGraphTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2919,7 +2919,7 @@ final class ModulesGraphTests: XCTestCase {
),
],
traits: [
.init(name: "Trait1", isDefault: true),
.init(name: "default", enabledTraits: ["Trait1"]),
"Trait1",
]
),
Expand Down Expand Up @@ -2955,7 +2955,8 @@ final class ModulesGraphTests: XCTestCase {
),
],
traits: [
.init(name: "Trait1", isDefault: true, enabledTraits: ["Trait2"]),
.init(name: "default", enabledTraits: ["Trait1"]),
.init(name: "Trait1", enabledTraits: ["Trait2"]),
.init(name: "Trait2", enabledTraits: ["Trait3", "Trait4"]),
"Trait3",
.init(name: "Trait4", enabledTraits: ["Trait5"]),
Expand Down Expand Up @@ -3004,7 +3005,7 @@ final class ModulesGraphTests: XCTestCase {
),
],
traits: [
.init(name: "Package1Trait1", isDefault: true),
.init(name: "default", enabledTraits: ["Package1Trait1"]),
"Package1Trait1",
]
),
Expand Down Expand Up @@ -3072,7 +3073,8 @@ final class ModulesGraphTests: XCTestCase {
),
],
traits: [
.init(name: "Package1Trait1", isDefault: true)
.init(name: "default", enabledTraits: ["Package1Trait1"]),
.init(name: "Package1Trait1")
]
),
Manifest.createFileSystemManifest(
Expand Down Expand Up @@ -3164,8 +3166,9 @@ final class ModulesGraphTests: XCTestCase {
),
],
traits: [
.init(name: "Package1Trait1", isDefault: true),
.init(name: "Package1Trait2", isDefault: true),
.init(name: "default", enabledTraits: ["Package1Trait1", "Package1Trait2"]),
.init(name: "Package1Trait1"),
.init(name: "Package1Trait2"),
]
),
Manifest.createFileSystemManifest(
Expand Down
Loading