Refactor property validation and simplify generation #26
Conversation
| @@ -68,13 +68,10 @@ jobs: | |||
| linux: | |||
| name: Build and Test on Linux | |||
| runs-on: ubuntu-latest | |||
| container: swift:5.9 | |||
There was a problem hiding this comment.
following the advice here to avoid this kind of CI failure
| case .ifConfigDecl: | ||
| return false | ||
| } | ||
| element.instantiableMacro != nil |
There was a problem hiding this comment.
I simplified our macro access while I was adding associated types to the Dependency.Source enum in order to share this code.
| @@ -35,7 +35,7 @@ public enum FixableInstantiableError: DiagnosticError { | |||
| public var description: String { | |||
| switch self { | |||
| case .dependencyHasTooManyAttributes: | |||
| "Dependency can have at most one of @\(Dependency.Source.instantiated), @\(Dependency.Source.received), or @\(Dependency.Source.forwarded) attached macro" | |||
| "Dependency can have at most one of @\(Dependency.Source.instantiatedRawValue), @\(Dependency.Source.receivedRawValue), or @\(Dependency.Source.forwardedRawValue) attached macro" | |||
There was a problem hiding this comment.
Dependency.Source is no longer a String-backed enum, so we need to use constants for these raw values now.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
==========================================
+ Coverage 99.27% 99.36% +0.09%
==========================================
Files 39 40 +1
Lines 8166 8888 +722
==========================================
+ Hits 8107 8832 +725
+ Misses 59 56 -3
|
| @@ -2521,7 +3172,7 @@ final class SafeDIToolTests: XCTestCase { | |||
| } | |||
| } | |||
|
|
|||
| func test_run_onCodeWithUnfulfillableRenamedReceivedPropertyType_throwsError() async { | |||
There was a problem hiding this comment.
I made the tests refer to Aliased rather than Renamed for consistency. We still talk about renaming in the README, but that's fine.
I'm also open to feedback that the case should be case renamed rather than case alias
| ) | ||
| } | ||
|
|
||
| func test_run_writesConvenienceExtensionOnRootOfTree_whenReceivedPropertyIsAliasedWasALevelBelowWhereItWasForwarded() async throws { |
There was a problem hiding this comment.
This test did already succeed, but it still seemed worth having this coverage.
dfed
force-pushed
the
dfed--refactor-property-resolution
branch
from
18dc6e5
to
842e32d
Compare
…r than re-validating during generation
dfed
force-pushed
the
dfed--refactor-property-resolution
branch
from
842e32d
to
41f8875
Compare
| instantiable: instantiable, | ||
| scope: instantiatedScope, | ||
| type: type | ||
| )) | ||
| } | ||
| scope.propertiesToInstantiate.append(contentsOf: additionalPropertiesToInstantiate) |
There was a problem hiding this comment.
propertiesToInstantiate might not be the best name. propertiesToGenerate?
| return !propertyStack.contains($0.property) && $0.property != property | ||
| case .received: | ||
| return false | ||
| } | ||
| } | ||
| .map(\.property) | ||
| ).subtracting( |
There was a problem hiding this comment.
do we need receivedProperties calculated for both cases? could simplify maybe
There was a problem hiding this comment.
ah we got rid of this anyways
This PR is best reviewed commit by commit, in split view, with whitespace ignored
There's a lot going on here, but the high level is that we are refactoring how we store and generate dependencies to remove a ton of potential issues.
We had a ton of little bugs because we were:
We fix both of these architectural issues in this PR (in separate commits), as well as a few other minor bugs that I found along the way.
Because we have updated the
Dependency.Source– whichSafeDIToolmay persist – this PR is a breaking change and we will need to roll to a 0.3.0 when this merges.