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

Add environment:get-parameters command #251

Merged
merged 1 commit into from
May 23, 2022

Conversation

MeKeyCool
Copy link
Contributor

@MeKeyCool MeKeyCool commented May 9, 2022

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
Copy link
Contributor

SebSept commented May 10, 2022

Thanks for contribution 🎉

Looks fine but :

@SebSept SebSept added the feature a new feature to implement label May 10, 2022
@MeKeyCool
Copy link
Contributor Author

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 ^^

@MeKeyCool MeKeyCool force-pushed the feature/add_getenv_command branch from 5a6e46d to 6449106 Compare May 10, 2022 08:42
@SebSept SebSept marked this pull request as draft May 10, 2022 10:27
@SebSept
Copy link
Contributor

SebSept commented May 10, 2022

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

@tom-combet
Copy link
Contributor

@SebSept It should be get_env instead of getenv.

@tom-combet
Copy link
Contributor

tom-combet commented May 10, 2022

@MeKeyCool Thanks for contribution!

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

config/services.yml Outdated Show resolved Hide resolved
@MeKeyCool MeKeyCool force-pushed the feature/add_getenv_command branch from 9c53371 to 2e7d6f6 Compare May 10, 2022 12:59
@MeKeyCool MeKeyCool changed the title WIP : Add environment:getenv command WIP : Add environment:get-env command May 10, 2022
@SebSept
Copy link
Contributor

SebSept commented May 10, 2022

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

@MeKeyCool MeKeyCool force-pushed the feature/add_getenv_command branch from 2e7d6f6 to 5a225f7 Compare May 10, 2022 15:03
@SebSept SebSept linked an issue May 10, 2022 that may be closed by this pull request
@MeKeyCool MeKeyCool force-pushed the feature/add_getenv_command branch from 5a225f7 to b2864b0 Compare May 11, 2022 14:24
@MeKeyCool MeKeyCool changed the title WIP : Add environment:get-env command Add environment:get-parameters command May 11, 2022
@MeKeyCool MeKeyCool marked this pull request as ready for review May 11, 2022 14:26
@SebSept SebSept requested review from nenes25 and SebSept May 12, 2022 08:55
Copy link
Contributor

@SebSept SebSept left a comment

Choose a reason for hiding this comment

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

Thanks @MeKeyCool for you first contribution :)
The command works fine and the parameters are in the help message, that's all good :)

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)

@MeKeyCool MeKeyCool force-pushed the feature/add_getenv_command branch from b2864b0 to d629e31 Compare May 16, 2022 08:12
@MeKeyCool MeKeyCool force-pushed the feature/add_getenv_command branch from d629e31 to 71642bd Compare May 16, 2022 08:20
@MeKeyCool MeKeyCool requested a review from SebSept May 16, 2022 08:21
@MeKeyCool
Copy link
Contributor Author

MeKeyCool commented May 16, 2022

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 ? ^^

Copy link
Contributor

@tom-combet tom-combet left a comment

Choose a reason for hiding this comment

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

OK for me! Tested and approved. ✔️

I see one easy improvement (but not necessary, maybe in an other pull request if someone needs it): add a 'key' option to get the value of one environment variable only. Like this:

bin/console fop:environment:get-parameters -k "database_host"

127.0.0.1

Copy link
Contributor

@SebSept SebSept left a comment

Choose a reason for hiding this comment

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

👍🏼 Thanks for the job and the patience !

@SebSept SebSept merged commit e80c602 into friends-of-presta:dev May 23, 2022
@MeKeyCool MeKeyCool deleted the feature/add_getenv_command branch May 23, 2022 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a new feature to implement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command to display informations about current shop state
3 participants