laravel / prompts

Beautiful and user-friendly forms for your command-line PHP applications.
https://laravel.com/docs/prompts
MIT License
532 stars 94 forks source link

Adding Facade For Calling The Functions #161

Closed TarsisioXavier closed 2 months ago

TarsisioXavier commented 2 months ago

The Feature 📦

In Laravel facades are a common thing to have, so I've made this small class which calls the functions of the helper file via static calls.

This PR adds a convenient way to call the functions without importing every function the dev uses in his script. I’ve also added a way to inject this facade class as an object - if the dev desires - and call the functions using “->”.

Possible Usages 📝

Before After
Before After
devajmeireles commented 2 months ago

Genius! But wouldn't this PR be aimed at adding only the Facade? I mean, maybe you don't need things as examples, after all it's the same as the same.

jessarcher commented 2 months ago

Hey @TarsisioXavier,

If you want to reduce the number of imports, you could import the Laravel\Prompts namespace and reference the functions as follows:

<?php

use Laravel\Prompts;

$name = Prompts\text('What is your name?');

Also, I'd avoid calling the class you've created a facade because it doesn't extend Laravel's Facade class so it doesn't have the facade features that the name implies.

devajmeireles commented 2 months ago

Hey @TarsisioXavier,

If you want to reduce the number of imports, you could import the Laravel\Prompts namespace and reference the functions as follows:

<?php

use Laravel\Prompts;

$name = Prompts\text('What is your name?');

Also, I'd avoid calling the class you've created a facade because it doesn't extend Laravel's Facade class so it doesn't have the facade features that the name implies.

Do you agree with the idea to have a Facade for this?

jessarcher commented 2 months ago

Do you agree with the idea to have a Facade for this?

I don't think I see a real need for the static class in this PR, especially with the namespace import already solving the multiple import issue mentioned in the PR description.

Having it as a real facade is potentially interesting for mocking prompts, but it's not something I've personally felt the need for. Additionally, we're trying to avoid depending on any illuminate/* packages in Prompts because we currently have a circular dependency with laravel/framework that is causing some grief each time we do a major release of Laravel.

TarsisioXavier commented 2 months ago

Genius! But wouldn't this PR be aimed at adding only the Facade? I mean, maybe you don't need things as examples, after all it's the same as the same.

Thanks for the compliment @devajmeireles. Yeah those files wore only for me to verify if everything behave the same way, I only wanted to be sure, so they are completely disposable.

TarsisioXavier commented 2 months ago

Good morning @jessarcher,

I did notice that you guys didn't make any facade and I was wondering why, so I tried my best to create one without the illuminate/support dependency. 😅
Even though it doesn't extend from Laravel facade class it follows the design pattern of a facade forwarding the function call to the "real" function. 🙂 Since there is no intention to add a facade for this package I guess we can close this PR then? 🥲

taylorotwell commented 2 months ago

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If applicable, please consider releasing your code as a package so that the community can still take advantage of your contributions!