Remove unused getPropertiesFromMethods method #109
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There might be one related side effect, so I'll discuss it here. This piece of docblock is taken from the generated models file: namespace App\Context\Product\Models{
/**
* …
*
* @method static \App\Context\Product\QueryBuilders\CourseQueryBuilder|Course newModelQuery()
* @method static \App\Context\Product\QueryBuilders\CourseQueryBuilder|Course newQuery()
* @method static \App\Context\Product\QueryBuilders\CourseQueryBuilder|Course query()
*/
class Course extends \Eloquent implements \App\Context\Product\Models\Concerns\WithCompliance, \App\Support\ActivityLog\ActivityLoggable, \App\Support\IsModel {}
}Notice how Psalm reports the following error: Now sure, -@method static \App\Context\Product\QueryBuilders\CourseQueryBuilder|Course query()
+@method static \App\Context\Product\QueryBuilders\CourseQueryBuilder query()So whenever the union type I think this is a standalone error, but it only popped up after using this PR's implementation of FakeModelCommand, so I think the broken implementation was hiding another bug. If someone can confirm that's probably the case, I'll make a separate issue. |
|
Yeah, I think the tests are legitimately failing. I don't have much free time right now to help with a solution, but I did this over at larastan a while ago larastan/larastan#479 Basically it parses the docblock and turns it into a typed collection instead |
|
@brendt so the docblock should just be |
|
I believe it should be |
|
Yeah, this is definitely a bug on Psalm's side |
|
Actually not a Psalm bug – the plugin is providing a generic stub for the parent class with the correct return type, and one for the child class generated via I resorted to a slightly nasty hack where I'm just removing the docblock-provided methods before Psalm has a chance to use them: acbfcaa#diff-9f7143a3a98cd6114dd5e4a17d64faa9R128 |
|
Awesome, will test this later today :) |
|
Hm, I've now encountered a fatal error when running psalm, but I'm not entirely sure whether it's related or not: I'm using the following commits: |
|
I'm happy to say all works now, thanks for the help both! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR fixes #98 .
The empty
getPropertiesFromMethodsmethod is clearly a bug, since it adds crucial information to the ide helper models file.By removing it, the original implementation is used again, and that works fine. I've tested this in our own Laravel project, and it fixes the issue described in #98.