-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
Proposal: Generic types for DTO casting #396
base: v3
Are you sure you want to change the base?
Conversation
@michaeldyrynda Have you tried with both Intelephense and Phpactor? My experience is that Phpactor works far better. It shouldn't be the reason, but I also realise that the return type of the DTO methods is mixed rather than object. In terms of changing this type, static analysis tools will start failing for those of us who use custom responses and override the DTO methods directly. I have actually managed to solve DTOs, by defining them in the request, and have them propagate to the DTO methods in the response. I can commit and push it if someone is interested anyway. |
Hey all! I was just looking through my blog post and agree that it was unecessary for me to add that parameter. I think it was a typo on my end that slipped through my proof reading haha 🤦 Although, I do like the idea of getting some kind of autocomplete and static analysis in my IDE, so I'd be a fan of this addition 🙂 |
Agree; I typically use resource-based SDK builds like the docs suggest, so do my typed returns on the connector, rather than on the request itself. I've tried with phpactor and it makes no difference sadly (though I'll be making the switch to that permanently regardless, it provides better diagnostics than intelephense!), which is a shame. Wish there was a better way of handling this, as having to annotate things by hand is grim. |
I did some tests with the proposed solution, and it works with Phpactor in Neovim, albeit with some adjustments. I set up a fresh Neovim using NvChad, and then just installed Phpactor. Interestingly enough, PhpStorm doesn't handle it at all. |
Well I'll be a monkey's uncle; I tried this again today and it seems to be working as per the PR. Unfortunately, making the change as @juse-less suggested raises the ire of PHPStan. So it would seem that the annotation is correct, and functions with both PHPStan and Intelephense. I believe there's a PHPStan plugin for PhpStorm that should enable intelligence for this? As it stands, the PR is correct, appeases static analysis, as well as completion for the two intelligence tools. Given that the annotations are PHPStan ones, I think it's reasonable for it to only work in that context. Whether or not this is considered breaking, however, is another story. |
Ohh. Right. /** @return ($dto is null ? object : T) */ I've always gotten complaints that the template has to be used on the param. In terms of PHPStan in PhpStorm, it's bundled and didn't make a difference in my screenshot. If it works as proposed, then I don't really see any issues with it per se. |
Hey @michaeldyrynda Thank you so much for this PR, I really like this idea and thank you for contributing too @juse-less ❤️ How do we feel about implementing a type check so if someone accidentally puts a different class in, it throws an error? Or even passing the class as a second argument like: createDtoFromResponse(Response $response, string $classString) The latter could open up cool DTO casting like |
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.
Awesome PR, thanks @michaeldyrynda!
Hmm this is really strange, the DTO type checking isn't working on my local machine (Running the latest PHPStorm 2024.1.2) was this actually still in discussion? |
@Sammyjo20 I wouldn't say it's a discussion. Adding this shouldn't break anything in PhpStorm, from I've seen. I'm not sure there's any tricks that can be done to make it work in PhpStorm. I can check the things I tested a while back, to see how it works in PhpStorm and Neovim. |
Thanks for the information @juse-less that's certainly a shame as it's the IDE I love to use. I'm still willing to release this PR as it is as it will help PHPStan analysis and as you mentioned, other editors. |
I switch between PhpStorm and Neovim quite a lot, depending on what I do, and where. The big upside of Michaels proposed solution is that it's so simple. The things I tried for v2, when we added more types and generics, got quite big and cumbersome to use in both Saloon itself, but also SDKs. |
@michaeldyrynda Can you just double-check that the changes proposed in the PR works? The reason I'm asking is because I tested them just now (your latest commit, without the style changes, from the diff view). Changes tested, and screenshot/**
* Cast the response to a DTO.
*
* @template T of object
*
* @return ($dto is class-string<T> ? T : object)
*/
public function dto(?string $dto = null): mixed As seen below, it won't suggest Intelephense:Phpactor:However, when I use my amended proposed changes, both of them works. Changes tested, and screenshot/**
* Convert the response into a DTO or throw a LogicException if the response failed
*
* @template T of object
*
* @param class-string<T>|null $dto
*
* @return ($dto is null ? object : T)
*/
public function dto(?string $dto = null): mixed As seen below, it suggests the Intelephense:Phpactor:I've tested with a fresh NvChad installation, as well as my own config. It still doesn't work for me in PhpStorm, though. In terms of PHPStan, I don't get any errors related to it. |
Hey @michaeldyrynda love this PR, any chance you could take a look at this comments and see if you can fix the IDE issues Juse raised? |
Just an idea - shouldn't it be possible to get the return type of the |
I wonder if this is worth testing with PHPStan v2 to see if there is any improvement? |
I've added annotations for the
dto
anddtoOrFail
methods, enabling passing of aclass-string
to each method, in order to get proper type suppose when working with the response.The new
$dto
parameter is optional to enable backwards-compatibility, but means that you can call the method with a FQCN passed as an argument, in order to get type completion.Omitting the argument continues as currently.
I was inspired by @ash-jc-allen's blog post, which passed the
FQCN
to this method, which initially looked unnecessary to me.public function getRepo(string $owner, string $repo): Repo { return $this->client() ->send(new GetRepo($owner, $repo)) + ->dtoOrFail(Repo::class); }
This implementation conforms to PHPStan requirements, so I believe it to be technically correct, but I wasn't actually able to verify it in my editor (Neovim).
For it to function (for me), I had to tweak the annotation slightly: