Skip to content
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.

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

Enable aliased received properties to be received again further down the chain #23

Closed
wants to merge 2 commits into from

Conversation

dfed
Copy link
Owner

@dfed dfed commented Jan 19, 2024

Once we renamed a property, we were not able to receive that renamed property further down the chain.

@dfed dfed self-assigned this Jan 19, 2024
Comment on lines -58 to +61
return $0.fulfillingProperty ?? $0.property
$0.fulfillingProperty ?? $0.property
case .forwarded,
.instantiated:
return nil
nil
Copy link
Owner Author

Choose a reason for hiding this comment

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

this is a style change. not related to the underlying fix here

Comment on lines +87 to +88
// If the dependency has a fulfilling property, the property has been aliased.
|| $0.fulfillingProperty != nil
Copy link
Owner Author

Choose a reason for hiding this comment

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

This matches similar code that we have over in the ScopeGenerator:

// If the source is not received, the property is being made available.
$0.source != .received
// If the dependency has a fulfilling property, the property is being aliased.
|| $0.fulfillingProperty != nil

Copy link
Owner Author

Choose a reason for hiding this comment

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

To be honest the more I think about this (and the PR built on top of this one)... the more I think the abstraction here is wrong. This change works and is mergeable, however I may take a bigger pass at cleaning this up before the next PR makes it out of draft.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah alright pushed up something way better in #24.

@dfed dfed requested a review from MrAdamBoyd January 19, 2024 01:08
@dfed dfed marked this pull request as ready for review January 19, 2024 01:08
Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (38c295c) 99.25% compared to head (12db159) 99.28%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
+ Coverage   99.25%   99.28%   +0.03%     
==========================================
  Files          37       37              
  Lines        8149     8304     +155     
==========================================
+ Hits         8088     8245     +157     
+ Misses         61       59       -2     
Files Coverage Δ
Sources/SafeDICore/Models/Scope.swift 100.00% <100.00%> (ø)
Tests/SafeDIToolTests/SafeDIToolTests.swift 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@dfed dfed marked this pull request as draft January 19, 2024 17:37
@dfed
Copy link
Owner Author

dfed commented Jan 19, 2024

Closing in favor of #26

@dfed dfed closed this Jan 19, 2024
@dfed dfed deleted the dfed--renamed-received-resolution branch January 19, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant