Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Refined ServiceManager factory, delegator and configuration types, to allow for easier introspection of mis-configuration at type level #98
Refined ServiceManager factory, delegator and configuration types, to allow for easier introspection of mis-configuration at type level #98
Changes from all commits
323e362
b12525b
3f0d8b2
1cd5bed
bd1e9a9
51f9cbe
007ae06
8e16a2a
6ba9e4d
5820225
d14e2eb
9dd34d7
9d3b97f
5b8b860
c4866a7
2ab2756
b906701
10f04b5
9e9746d
883ff57
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This coercion shouldn't apply anymore:
merge()
has the correct types? 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArrayUtils::merge
returnsarray
and thus, psalm loses track.Not sure if something like this would help psalm to keep track:
But this wont properly work for nested arrays (as they are here). There is absolutely no way but to trust that
ArrayUtils::merge
will return the same types.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but you are not calling
ArrayUtils::merge()
, but rather$this->merge()
, which is well typed, and has its own suppressionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave this alone for now - I think it's not worth wasting effort on it after all the work you've already put in this, and it's in the internals anyway :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added
reportUnusedPsalmSuppress
in the config and thats not reported so something might still be odd :-/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is supposed to return
object
, then$callback
cannot returnmixed
.I feel like the
DelegatorFactoryInterface
may benefit from a templated type, in future (not here, due to BC)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
ContainerInterface#get
can returnmixed
, this actually has to bemixed
:-(But yes, having a templated type would help for sure but until then, I've thought that
mixed
would be fine until the return type is object 🤷🏼♂️There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but the commented line is about
@return object
(wrong)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I can follow.
Do you want me to change existing documentation that we end up having bc breaks? as of now, due to the lack of typing, callable factories can return mixed. When I enforce
object
now, this will be pain probably. Thats why I think that might be a BC break but I am not 100% sure tho.Or do you want me to open up the return value of the delegator to be mixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am fine with both as I don't have that much experience with adding psalm types on such a widely used component. As always, I trust your experience. :-)