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

Feature | Choose HTTP method when using saloon:request command #50

Merged
merged 6 commits into from
Dec 2, 2023

Conversation

JonPurvis
Copy link
Contributor

@JonPurvis JonPurvis commented Sep 30, 2023

Hey 👋

Whenever I create a Request class, unless it's a GET request, I need to go into the class and change the method to the type I need it to be. I thought it might be nice to ask what method type it should be, before the class is created, kinda how we ask for the Integration and Name of it.

This PR adds a new option when creating a Request class, to be able to specify the type of method, the request is and because it makes use of Laravel Prompts, this slots in to the current flow rather nicely:

Monosnap test – WinchesterWebsitesCommand php 2023-09-30 16-40-58

The MakeRequest class will then match the selected method to a stub file, or default to the already existing saloon.request.stub file.

This is targeting v3 as it makes use of the resolveStubName method that was added in #45

Happy to make any changes that are needed, to get this merged! 🌵

@JonPurvis
Copy link
Contributor Author

JonPurvis commented Sep 30, 2023

Hey @Sammyjo20 👋

Ash reminded me that Laravel Prompts were introduced in Laravel 10, so any Laravel 9 apps running this package won't be able to use this change, in fact, I tested it on a Laravel 9 application, and I get:

Call to undefined function Laravel\Prompts\select()

Happy to make the change to get it work with Laravel 9, however, would you be open to discussing the possibility of bumping the illuminate/console package so the minimum is 10? Especially with v3 being a major release? It'd be super cool to be able to use Prompts with this package!

🤠

@Sammyjo20
Copy link
Member

Hey, Jon! Awesome PR 🤠 Thank you! This is such a good idea, as it's always something you have to change when a request class has been made.

With regards to Laravel Prompts - if we bumped to a minimum of L10 now, it would restrict anyone wanting to upgrade to v3's pagination plugin until they upgraded to Laravel 10. I'm thinking what might be more developer-friendly is merging this (and bumping the version) and releasing it as a minor 3.1 release shortly after. What do you think? Composer is really clever in this scenario and if it detects anyone on Laravel 9, the highest they'll be able to go is 3.0, so your 3.1 feature can then be for people using Laravel 10+.

@Sammyjo20
Copy link
Member

The other thought I have is if there was a way to have a single stub file but insert the request type as a variable, similar to how we inject the class name? If that could be achieved then you would only need one stub file.

@Sammyjo20 Sammyjo20 changed the title add option for method type Feature | Offer choice of HTTP method when using saloon:request command Sep 30, 2023
@JonPurvis
Copy link
Contributor Author

@Sammyjo20 Hey Sam!

I'm happy for this to be released as 3.1, it definitely makes sense with what you said, for sure! Last thing I want to do is cause a negative dev experience when using Saloon!

As for the single stub file, this was actually my original intention but couldn't figure out the best way to do it. I revisited it after a break and we now have just 1 stub file! 🥳 It now has {{ method }}, instead of GET, which we replace by overriding the buildClass() method in the MakeRequest class with whatever Method is selected by the user during the creation.

Thanks!

Copy link
Member

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

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

This is 🥵 @JonPurvis! I haven't yet tested this on my testbench but code looks good! I'll wait until v3 has launched cough tomorrow 😉

Also you might want to keep this as a Hactoberfest contribution!

@Sammyjo20 Sammyjo20 changed the title Feature | Offer choice of HTTP method when using saloon:request command Feature | Choose HTTP method when using saloon:request command Oct 1, 2023
@JonPurvis
Copy link
Contributor Author

Thanks @Sammyjo20 🥳

This has actually given me some more ideas on things we can do when you're creating your classes, but I'll wait for 3.1 to be released first!

@Sammyjo20
Copy link
Member

Hey @JonPurvis awesome PR - I think the only thing you need is to composer require laravel/prompts? Because it's not installed in Laravel by default and anyone who doesn't have it installed will get an error.

I'm also wondering that because Laravel prompts is still <1.0.0 will it cause issues with people trying to install it with conflicting versions? Or should we require it like "<1.0.0" instead of ^0.1.0?

@JonPurvis
Copy link
Contributor Author

Hey Sam!

I'm actually pretty sure that Laravel Prompts comes with Laravel 10. I've just checked the composer.json file of the laravel/framework package in my shiny new Laravel 10 application and it is in there:

"laravel/prompts": "^0.1.9"

When I run php artisan saloon:request on this branch in my application, I get Laravel Prompts, without having to install the package myself:

image

Actually, just checked the Laravel Prompts documentation and it does state:

Laravel Prompts is already included with the latest release of Laravel.

So I think we should be already covered?

Feel free to correct me if I'm wrong though, of course! 😆

@JonPurvis
Copy link
Contributor Author

Hey @Sammyjo20 👋

I've had a look at the status checks on this PR. Is there something I need to do to get the code style one to pass? The actual error doesn't seem to be an issue with the style, more to do with a GitHub action that I assume gets ran as part of the checks.

If it is something I can sort, please let me know!

🤠

@Sammyjo20
Copy link
Member

Hey @JonPurvis I'm so sorry for the delay on this PR - it's been mad but I think my head is finally back in the Saloon once again 🤠

You can fix that locally by running composer fix-code and that should clear anything up. I'll take another look at the code now!

@JonPurvis
Copy link
Contributor Author

Hey @Sammyjo20

Don't worry about it! You've been busy getting that XML Wrangled! 🤠

Thanks for that, I've ran that on this branch and pushed the update and it looks to have resolved it!

@JonPurvis
Copy link
Contributor Author

Awesome, all passing now! 🥳

@Sammyjo20
Copy link
Member

Awesome! I've just had a look and I'm happy with the code but when I ran it locally to test it seemed to have some strange behaviour. I tried to work it out but I'm a little rusty with the ol generator commands :D

Basically, if I write:

php artisan saloon:request

It will work, and it'll correctly prompt for the method, however if I write

php artisan saloon:request Forge GetServersRequest

I'll actually get an error, and it won't prompt me for the method. Do we need to just provide a default method here? I've pushed a commit - let me know if this is the wrong kind of fix.

@JonPurvis
Copy link
Contributor Author

Hey,

That's absolutely my bad. I'm 99% sure I only actually ran saloon:request when doing the initial pass of this PR 😆

I've just pulled down your changes and can confirm they work, setting it to GET by default.

Thanks for sorting that, my apologies!

@Sammyjo20
Copy link
Member

No problem at all, it's nice to be able to help! Thanks for this awesome feature! Get the tweets at the ready!

@Sammyjo20 Sammyjo20 merged commit 29f653b into saloonphp:v3 Dec 2, 2023
9 checks passed
@JonPurvis
Copy link
Contributor Author

Glad to have been able to work on it!

Now time for some Rum, to celebrate! 🍻

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.

2 participants