colcon / colcon-cd

A shell function for colcon to change the current working directory
http://colcon.readthedocs.io
Apache License 2.0
4 stars 4 forks source link

Add colcon_cd bash completer #14

Closed Briancbn closed 8 months ago

Briancbn commented 2 years ago

Address #12

I had some time to work on this and managed to add in the auto-completion feature for colcon_cd. The solution is a bit hackish, but I hope it will be helpful.

How to use

. <path-to-workspace>/install/colcon-cd/share/colcon_cd/function/colcon_cd.sh
. <path-to-workspace>/install/colcon-cd/share/colcon_cd/function/colcon_cd-argcomplete.bash
# . <path-to-workspace>/install/colcon-cd/share/colcon_cd/function/colcon_cd-argcomplete.zsh

My original intention is to make use of argcomplete's --external-argcomplete-script to create a fake console command (entrypoint name: _colcon_cd_completions) and use that console command for bash completion. This is implemented in completer.py.

However, due to the argcomplete version differences between bootstrap setup pip (1.12.3) and Ubuntu 20.04 native python3-argcomplete (1.8.1), I have to include the entire bash script from argcomplete 1.12.3, and also not being able to remove the auto-completion for unused variable -- base_path (ref).

I have verified that it works with bash on Ubuntu 20.04 native and bootstrap setup, I am not familiar with zsh testing, it will be helpful if I could get help from the maintainers on this.

Tests pass with colcon test locally, CI pending https://github.com/colcon/colcon-cd/pull/13, passed after rebase

It might be easier way to do this completely in shell, but I just cannot figure it out and this was what I currently decide to go with.

Briancbn commented 2 years ago

Rebased to latest, ready for review!

Briancbn commented 2 years ago

Pinging here again. Please help review this if anybody have time..

MatthijsBurgh commented 1 year ago

@Briancbn is this PR still up-to-date to be merged?

@cottsay Please merge this one, when all issues are addressed.

MatthijsBurgh commented 1 year ago

@cottsay friendly ping to review (and merge) this PR.

cottsay commented 1 year ago

A couple of thoughts:

  1. Do you intend to distribute this extra functionality on an opt-in basis? If we wanted this to be enabled "by default", the dependency on argcomplete would need to be declared in the setup.cfg and stdeb.cfg and the new hooks should be added to the colcon.pkg file. I'm not sure that's the right thing to do, especially given that this doesn't work on batch/windows shells, but I thought I'd highlight it.
  2. Is it possible to use register-python-argcomplete instead of creating the completion script manually like colcon-argcomplete does?
cottsay commented 1 year ago

CI is broken for now, I'll get that cleaned up this week so we can get a green build here.

Briancbn commented 1 year ago
  1. Do you intend to distribute this extra functionality on an opt-in basis

Yes, only when sourcing the bash. My reason:

  1. argcomplete quite slow since it runs the package discovery. (unlike roscd)
  2. consistent with the rest of colcon.

    If there is no plan to support Windows here in the future, I believe the "by default" makes sense.

2. Is it possible to use register-python-argcomplete instead of creating the completion script manually like colcon-argcomplete does?

Looks promising. Let me verify.

Is there any reason that colcon-argcomplete didn't choose to use register-python-argcomplete? I would assume it's best to keep the autocompletion setup consistent within colcon repos.

MatthijsBurgh commented 1 year ago

@cottsay friendly ping ;)

cottsay commented 1 year ago

CI is broken for now

CI is now fixed.

Is there any reason that colcon-argcomplete didn't choose to use register-python-argcomplete?

It does: https://github.com/colcon/colcon-argcomplete/blob/master/completion/colcon-argcomplete.bash

MatthijsBurgh commented 1 year ago

Lets merge this thing and release it please.

MatthijsBurgh commented 1 year ago

@cottsay Sorry, I thought the only blocking thing was the broken CI.

Briancbn commented 1 year ago

I will work on this next weekend. Sorry been busy with other things lately. @cottsay thanks for the suggestion!

Briancbn commented 1 year ago

@cottsay Thanks for the guidance. Works like charm! This is really brilliant.

To elaborate on why I suggested that might be a good idea, here's a simple change that could eliminate the copy/paste from shell_integration.py.

diff --git a/function/colcon_cd.sh b/function/colcon_cd.sh
index d85751b1aea4..3d1f961a952c 100644
--- a/function/colcon_cd.sh
+++ b/function/colcon_cd.sh
@@ -1,7 +1,9 @@
 # copied from colcon-cd/function/colcon_cd.sh

 colcon_cd() {
-  if [ $# = 0 ]; then
+  if [ "$_ARGCOMPLETE" == "1" ]; then
+    python3 -m colcon_cd.completer
+  elif [ $# = 0 ]; then
     # change the working directory to the previously saved path
     if [ "$_colcon_cd_root" = "" ]; then
       echo "No previous path saved. Either call 'colcon_cd <pkgname>' from a" \

I am still not entirely sure what is happening here. I think the above changes including the python3 -m colcon_cd.completer part gets executed within here (https://github.com/kislyuk/argcomplete/blob/v1.8.1/argcomplete/bash_completion.d/python-argcomplete.sh), Please correct me if I am wrong.

Briancbn commented 11 months ago

@cottsay pinging again. Ready to review if you have time.