|
|
Patch Set 1 #
Total comments: 6
Patch Set 2 : Try to find modules via python script #
Total comments: 8
Patch Set 3 : Fix typo #Patch Set 4 : Added completion for client #Patch Set 5 : Better variable naming #Total messages: 13
|
ced
http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion File etc/trytond_completion (right): http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion#newcode1 etc/trytond_completion:1: Missing shebang
|
14 years, 12 months ago (2011年01月13日 22:49:43 UTC) #1 |
http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion File etc/trytond_completion (right): http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion#newcode1 etc/trytond_completion:1: Missing shebang
http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion File etc/trytond_completion (right): http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion#newcode1 etc/trytond_completion:1: On 2011年01月13日 22:49:43, ced wrote: > Missing shebang I checked and no other script in /etc/bash_completion.d/ comes with a shebang. And it's logical because the file is sourced and never called directly.
Nice. Where should it go in the trytond package? http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion File etc/trytond_completion (right): http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion#newcode1 etc/trytond_completion:1: On 2011年01月13日 23:03:46, bch wrote: > On 2011年01月13日 22:49:43, ced wrote: > > Missing shebang > > I checked and no other script in /etc/bash_completion.d/ comes with a shebang. > And it's logical because the file is sourced and never called directly. Some have, some not, so it seems to be deliberate. http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion#newcode18 etc/trytond_completion:18: --update) i.e. -u|--update for the short versions http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion#newcode19 etc/trytond_completion:19: module_path="$(dirname $(which ${pgm}))"/../trytond/modules/ JFTR: path will have to be patched for debian, don't know for others distribution http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion#newcode20 etc/trytond_completion:20: modules=$(find ${module_path} -maxdepth 1 -type d -printf "%f\n") #FIXME FIXME for modules/ itself contained in the listing?
On 2011年01月14日 12:40:12, yangoon wrote: > Nice. Where should it go in the trytond package? Sorry didn't read your questions on tryton-dev. > http://codereview.appspot.com/3997041/diff/1/etc/trytond_completion#newcode19 > etc/trytond_completion:19: module_path="$(dirname $(which > ${pgm}))"/../trytond/modules/ > JFTR: path will have to be patched for debian, don't know for others > distribution dito
http://codereview.appspot.com/3997041/diff/6001/trytond_completion File trytond_completion (right): http://codereview.appspot.com/3997041/diff/6001/trytond_completion#newcode14 trytond_completion:14: PYTHON_EXEC=$(which python2 2>/dev/null) python2 doesn't exist on every OS http://codereview.appspot.com/3997041/diff/6001/trytond_completion#newcode45 trytond_completion:45: opts="--version -h --help -c --config --debug -v --verbose -d --database -u --update --pidfile --logfile -i --init" Perhaps you can find it with the output of `trytond --help`
http://codereview.appspot.com/3997041/diff/6001/trytond_completion File trytond_completion (right): http://codereview.appspot.com/3997041/diff/6001/trytond_completion#newcode14 trytond_completion:14: PYTHON_EXEC=$(which python2 2>/dev/null) On 2011年01月19日 08:38:44, ced wrote: > python2 doesn't exist on every OS This is why the code first try to find it and if it does not exist user python (lines 15-18). http://codereview.appspot.com/3997041/diff/6001/trytond_completion#newcode45 trytond_completion:45: opts="--version -h --help -c --config --debug -v --verbose -d --database -u --update --pidfile --logfile -i --init" On 2011年01月19日 08:38:44, ced wrote: > Perhaps you can find it with the output of `trytond --help` IMO it's better to hardcode them: the options does not change every day and it will had a lot of code with potential bugs.
http://codereview.appspot.com/3997041/diff/6001/trytond_completion File trytond_completion (right): http://codereview.appspot.com/3997041/diff/6001/trytond_completion#newcode14 trytond_completion:14: PYTHON_EXEC=$(which python2 2>/dev/null) On 2011年01月19日 09:08:26, bch wrote: > On 2011年01月19日 08:38:44, ced wrote: > > python2 doesn't exist on every OS > > This is why the code first try to find it and if it does not exist user python > (lines 15-18). Even python must not exist on every system. It is not required for a base install of debian. http://codereview.appspot.com/3997041/diff/6001/trytond_completion#newcode17 trytond_completion:17: PYTHON_EXEC=$(which python2 2>/dev/null) do you mean here: PYTHON_EXEC=$(which python 2>/dev/null) ?
http://codereview.appspot.com/3997041/diff/6001/trytond_completion File trytond_completion (right): http://codereview.appspot.com/3997041/diff/6001/trytond_completion#newcode14 trytond_completion:14: PYTHON_EXEC=$(which python2 2>/dev/null) > Even python must not exist on every system. It is not required for a base > install of debian. In this case this script will not be installed. http://codereview.appspot.com/3997041/diff/6001/trytond_completion#newcode17 trytond_completion:17: PYTHON_EXEC=$(which python2 2>/dev/null) On 2011年01月19日 10:46:52, yangoon wrote: > do you mean here: > PYTHON_EXEC=$(which python 2>/dev/null) > ? Oups, bad copy-paste.
The completion for client provides completion on log channels (currently "common view rpc.request rpc.result common.options all"), but I don't know if it's correct.