[Traits] Change TraitConfiguration to be an enum instead of struct
#8370
Conversation
To phase in the modification to TraitConfiguration, start with the new enum and match the inits configured for the legacy TraitConfiguration struct
…method * enabledTraits will now instead default to using the new trait config * comment out enabledTraits legacy method in favour of enabledTraits2; begin using this method instead. To rename to enabledTraits. * remove optional TraitConfiguration parameters, since enum encapsulates a "no configuration" case. TODO: * Must still clean up commented out code * Ensure behaviour still operates as expected * Add tests for calculateAllEnabledTraits, as this is now public * Ensure checks on TraitConfiguration aren't redundant i.e. make appropriate use of the new structure * Pre-calculate enabled traits across the root with the TraitConfiguration and store where appropriate so we don't have to recalculate this constantly
- Since we will calculate all enabled traits for root manifests in the PackageGraphRoot init (which can throw depending on whether there is a non-allowable trait configuration passed), allow the init to throw this error as well. - Clean up some commented code - Replace redundant calculations of enabled traits since we now store the calculated traits in the PackageGraphRoot TODO: - Address disallow disabling traits test not catching error - More cleanup of commented out code
| switch self { | ||
| case .enabledTraits(let traits): | ||
| return traits | ||
| case .noConfiguration, .enableAllTraits: |
There was a problem hiding this comment.
thought: it seems odd to me that the .enableAllTraits returns nil. Should it return a set with all the traits instead?
There was a problem hiding this comment.
Since .enableAllTraits represents a flag that a user would pass in (and as such the TraitConfiguration is constructed before we begin parsing the Manifest which is where we'd have access to the entire list of traits that a manifest defines), that was the reasoning for it returning nil at this stage. Though since the enum now allows me to do specific checks for these cases where this information would be relevant, there may not be a need for this computed property - haven't fully refactored everything just yet.
There was a problem hiding this comment.
This has made me notice that it would probably make more sense to have .noConfiguration (or rather .none as the updated case name) return ["default"] here. :)
| package var enableAllTraits: Bool | ||
| public enum TraitConfiguration: Codable, Hashable { | ||
| case enableAllTraits | ||
| case noConfiguration |
There was a problem hiding this comment.
question: what does noConfiguration represent? how is it different than the noEnabledTraits? is there an actual behavioural difference between noConfiguration and noEnabledTraits?
There was a problem hiding this comment.
There is a difference in behaviour, yes: I've since changed the nomenclature for these cases and will be pushing that up today, but essentially noConfiguration encapsulates the case where there is no configuration of traits passed in by the user and as such will default to using a package's default traits if there are any. If a user does explicitly pass in a configuration with an empty set of traits enabled (so essentially, no traits enabled), this overrides the default traits behaviour and instead disables all traits which is what .noEnabledTraits accomplishes. :)
| } else { | ||
| // Since enableAllTraits isn't enabled and there isn't a set of traits set, | ||
| // there is no configuration passed by the user. | ||
| self = .noConfiguration |
There was a problem hiding this comment.
question: could we have also set .noEnabledTraits to get the same behaviour?
- Updated the test that asserts for an error thrown when disabling
default traits for a package with no traits; prior, there was a
chance to encounter a false positive if there was no error
being thrown due to the do/catch block. Now, we assert for an
error being thrown and check that it's the expected error message.
- Changed the case names for TraitConfiguration:
- noConfiguration -> none
- noEnabledTraits -> disableAllTraits
- Added optional parameter to calculateAllEnabledTraits entitled
'parentPackage' that keeps a reference to the parent package
(if any) when computing the enabled traits of a package; this
way, we can meaningfully report an error if a parent package
sets an unallowable set of traits to be enabled/disabled for
a dependency.
- Move the trait helper and validation methods to a separate file (Manifest/Manifest+Traits.swift) and move the TraitConfiguration into the PackageModel instead of PackageGraph; updated the appropriate CMakeLists.txt - Update TraitConfiguration computed variable for enabled traits; '.none' case should return '["default"]', as that is what its behaviour defaults to. Removed other computed variables in favor of checking the enum cases themselves where appropriate.
- To support further testing on traits and their possible configurations, set traits where appropriate in the MockDependency + make PackageDependency.Trait public to allow its use in the mocks. - Fixed up the error messages displayed for traits - Added more tests to cover various possible configurations of traits
|
@swift-ci please test |
Fixes #8355.
Since the way a
TraitConfigurationis set up can affect how traits are calculated and whether default traits are overridden or not, it's more ergonomic to change this to anenumthat lists the various possible cases of configuration:This way we can avoid doing multiple checks on the properties of the
structand instead rely on the exact case that describes how to continue calculating enabled traits.The refactoring done as a part of this change also fixes #8356.