Added Magento2 CLI completions by Radiergummi · Pull Request #4043 · fish-shell/fish-shell

Radiergummi · 2017-05-18T11:39:46Z

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 · 2017-05-18T12:09:55Z

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.

faho · 2017-05-18T12:11:38Z

+################################################
+
+  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) ]


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.

faho · 2017-05-18T12:12:34Z

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


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

faho · 2017-05-18T12:13:48Z

+    set -l users (__fish_print_magento_mysql_command 'select user from mysql.user;' | sort -u)
+
+    for i in $users
+      echo $i\t"$i"


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.

faho · 2017-05-18T12:14:13Z

+# 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)


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

faho · 2017-05-18T12:14:58Z

+  set -l modules (magento module:status)
+
+  for i in $test
+    if [ (echo $i) -a (echo $i) != 'None' ]


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

faho · 2017-05-18T12:24:16Z

+#
+# 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)";


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

Taken from the original help output.

faho · 2017-05-18T12:24:36Z

+#
+# 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.";


"... or omit for all".

Taken from the original help output.

faho · 2017-05-18T12:26:40Z

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


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'

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

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

faho · 2017-05-18T12:27:58Z

+#
+# list
+#
+__fish_magento_register_command_option list -f -l xml -d "To output list as XML";


"Output as XML".

Taken from the original help output.

faho · 2017-05-18T12:28:52Z

+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";


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

Same as above, taken from Magento help output

faho · 2017-05-18T12:31:52Z

-#
-# 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.';


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

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

You have? Sorry, I must have missed that!

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

faho · 2017-05-18T15:06:03Z

Merged, thanks!

@zanchey: This is good for 2.6.0.

* 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 · 2017-05-19T08:47:50Z

Thanks, taken for 2.6.0 as 291d88a.

Removed functions using MySQL, updated formatting

ed8be55

faho requested changes May 18, 2017

View reviewed changes

faho reviewed May 18, 2017

View reviewed changes

Radiergummi added 2 commits May 18, 2017 15:30

Several smaller fixes

b86c97c

Improved descriptions

fcf58f9

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

faho approved these changes May 18, 2017

View reviewed changes

faho added completions enhancement labels May 18, 2017

faho added this to the 2.6.0 milestone May 18, 2017

faho merged commit 71f5fe1 into fish-shell:master May 18, 2017

github-actions Bot locked as resolved and limited conversation to collaborators Apr 17, 2020

Uh oh!

Conversation

Radiergummi commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODOs:

Uh oh!

faho commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

faho commented May 18, 2017

Uh oh!

zanchey commented May 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Radiergummi commented May 18, 2017 •

edited

Loading