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

Added Magento2 CLI completions #4043

Merged
merged 4 commits into from May 18, 2017
Merged

Conversation

Radiergummi
Copy link
Contributor

@Radiergummi Radiergummi commented May 18, 2017

Description

This is the completion file for the Magento2 CLI application I use on my servers. It has an additional feature tho, I'm not sure if it fits into the fish completion philosophy:
If you provide limited access credentials, it will connect to the MySQL database and provide additional suggestions, such as available users, themes or indexers in the database. If this file is never touched, those suggestions simply won't show up. I, personally, find them to be pretty useful, though.

Should I remove those database suggestions before creating a PR?

Fixes issue #

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

This is the completion file for the Magento2 CLI application I use on my servers. It has an additional feature tho, I'm not sure if it fits into the fish completion philosophy:  
If you provide limited access credentials, it will connect to the MySQL database and provide additional suggestions, such as available users, themes or indexers in the database. If this file is never touched, those suggestions simply won't show up. I, personally, find them to be pretty useful, though.  

Should I remove those database suggestions before creating a PR?
@faho
Copy link
Member

faho commented May 18, 2017

Should I remove those database suggestions before creating a PR?

Yes, please. Approximately one person (i.e. you) is going to use that, so the code will just rot.

I'll add other comments inline.

################################################

set -l result (mysql -u{$fish_mysql_user} -p{$fish_mysql_pass} -e $argv --skip-column-names 2>&1 | grep -v "mysql:")
if [ (echo $result | grep ERROR) ]
Copy link
Member

Choose a reason for hiding this comment

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

As a general comment (this will be remove later):

  • We prefer string match for this since it doesn't require forking off an external process

  • The test is unnecessary since both grep and string match have a "-q" option that silences output and returns 0 if there are matches, 1 if there are none.

So this would be if string match -qr 'ERROR' -- $result or if echo $result | grep -q ERROR.


function __fish_print_magento_url_protocols -d "Shows all available URL protocols"
echo http://\t"HTTP"
echo https://\t"HTTPS"
Copy link
Member

Choose a reason for hiding this comment

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

In general, if suggestions just duplicate information you can already see we prefer leaving them. So this would just echo "http://" and "https://".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

set -l users (__fish_print_magento_mysql_command 'select user from mysql.user;' | sort -u)

for i in $users
echo $i\t"$i"
Copy link
Member

Choose a reason for hiding this comment

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

Like above, these descriptions are completely useless.

So this entire loop can just be printf '%s\n' $users. Alternatively, just let the output of the above command substitution go out.

# Retrieves all available MySQL users
###
function __fish_print_magento_mysql_users -d "Shows all available MySQL users"
set -l users (__fish_print_magento_mysql_command 'select user from mysql.user;' | sort -u)
Copy link
Member

Choose a reason for hiding this comment

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

Sorting and removing duplicates is something fish already does, so you can leave off the sort -u.

set -l modules (magento module:status)

for i in $test
if [ (echo $i) -a (echo $i) != 'None' ]
Copy link
Member

Choose a reason for hiding this comment

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

The echos here are useless. if test -n "$i" -a "$i" != "None".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

#
# maintenance:disable
#
__fish_magento_register_command_option maintenance:disable -l ip -d "Allowed IP addresses (use 'none' to clear allowed IP list) (multiple values allowed)";
Copy link
Member

Choose a reason for hiding this comment

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

Because of the plural, it already sounds like multiple values are okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taken from the original help output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

#
# cache:clean
#
__fish_magento_register_command_option cache:clean -f -a "(__fish_print_magento_cache_types)" -d "Space-separated list of cache types or omit to apply to all cache types.";
Copy link
Member

Choose a reason for hiding this comment

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

"... or omit for all".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taken from the original help output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

#
# admin:user:create
#
__fish_magento_register_command_option admin:user:create -f -r -l admin-user -d "(Required) Admin user";
Copy link
Member

Choose a reason for hiding this comment

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

Since these are required, it would be great if we could just automatically suggest them. That currently doesn't happen with options, so you'll have to suggest one via "-a".

So that would be

# Force suggesting options
__fish_magento_register_command_option admin:user:create -a '--admin-user'

Copy link
Member

Choose a reason for hiding this comment

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

(Note: Ideally you'd suggest only the ones that haven't been given yet. See __fish_contains_opt)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I have a question though: Is it possible to indicate a parameter needs a value? Magento requires parameter values as --param="value", but I haven't found a way to insert the equals sign yet..

#
# list
#
__fish_magento_register_command_option list -f -l xml -d "To output list as XML";
Copy link
Member

Choose a reason for hiding this comment

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

"Output as XML".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taken from the original help output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

complete -x -c magento -o -vv;
complete -x -c magento -o vvv;
complete -x -c magento -s V -l version -d "Display this application version";
complete -x -c magento -l ansi -d "Force ANSI output";
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming the "ANSI" here refers to ANSI color codes? Then it should be "Force colored output".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, taken from Magento help output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

#
# indexer:reindex
#
__fish_magento_register_command_option indexer:reindex -f -a "(__fish_print_magento_indexer_names)" -d 'Space-separated list of index types or omit to apply to all indexes.';
Copy link
Member

Choose a reason for hiding this comment

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

You probably still want to complete these commands, even if you can't complete the arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well they don't have any parameters, and I've registered the subcommand itself already, so that should be fine, shouldn't it?

Copy link
Member

Choose a reason for hiding this comment

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

You have? Sorry, I must have missed that!

Tried to shorten the text as much as possible and removed unnecessary characters
@faho faho added this to the 2.6.0 milestone May 18, 2017
@faho faho merged commit 71f5fe1 into fish-shell:master May 18, 2017
@faho
Copy link
Member

faho commented May 18, 2017

Merged, thanks!

@zanchey: This is good for 2.6.0.

zanchey pushed a commit that referenced this pull request May 19, 2017
* Added Magento2 CLI completions

This is the completion file for the Magento2 CLI application I use on my servers. It has an additional feature tho, I'm not sure if it fits into the fish completion philosophy:
If you provide limited access credentials, it will connect to the MySQL database and provide additional suggestions, such as available users, themes or indexers in the database. If this file is never touched, those suggestions simply won't show up. I, personally, find them to be pretty useful, though.

Should I remove those database suggestions before creating a PR?

* Removed functions using MySQL, updated formatting

* Several smaller fixes

* Improved descriptions

Tried to shorten the text as much as possible and removed unnecessary characters

(cherry picked from commit 71f5fe1)
@zanchey
Copy link
Member

zanchey commented May 19, 2017

Thanks, taken for 2.6.0 as 291d88a.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants