friends-of-presta / fop_console

Prestashop Module providing a set of shell/terminal commands for developers (PrestaShop 1.7.5+)
Academic Free License v3.0
84 stars 35 forks source link

Add environment:get-parameters command #251

Closed MeKeyCool closed 2 years ago

MeKeyCool commented 2 years ago

I am currently working on environment variables for Prestashop and I need an easy command to make my development / tests easier.

This command is under work but I let it there to receive comments or ideas for improvements ;) If it is a problem, I can close it :)

SebSept commented 2 years ago

Thanks for contribution 🎉

Looks fine but :

MeKeyCool commented 2 years ago

Thanks for contribution tada

Looks fine but :

* The command name / service name / classname must be consistent ( https://github.com/friends-of-presta/fop_console/runs/6364780235?check_suite_focus=true) - Currently there no tool to help you naming it, you should look at the existing ones to get information ([DevTool : command to generate Class name, service name from a command name. #212](https://github.com/friends-of-presta/fop_console/issues/212)) - (This should also be written in our contributing file).

* just a question : how does it behave it an env value is not defined ?

Sorry it is still a draft but yes I will make it safier (testing if parameter exists before getting it)

Ok, I will add some comments/descriptions to the command ^^

For naming / standards, I'll try to understand and fix problem returned by CI ^^

SebSept commented 2 years ago

@tom-combet do you have a minute for the service name ? It looks correct but failed ... (I'll check too, later).

tom-combet commented 2 years ago

@SebSept It should be get_env instead of getenv.

tom-combet commented 2 years ago

@MeKeyCool Thanks for contribution!

I'll review it next week, too busy right now sry...

SebSept commented 2 years ago

service name must be fop.console.environment.get_env.command (I've made a suggestion, you can commit here on github. ).

MeKeyCool commented 2 years ago

Undefined values are spotted with a red background, that good. I suggest, you do the same for boolean values. Currently it's not possible to know if a configuration is false or an empty string, maybe you can render the boolean true and false with or style.

The other point it that it's can be better to have a try/catch block to avoid unclear errors that may happen (but will probably never happen). (not mandatory, but better imo)

I updated output styling. As you let me the choice, I don't care unclear errors cause in command line I don't think this problem would be critical nor hard to solve.

Is it ok for you this way ? ^^