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

roszsh: Ignore hidden files and directory in _roscomplete_exe. #100

Conversation

Projects
None yet
2 participants
@de-vri-es
Copy link
Contributor

commented Dec 23, 2015

This PR filters hidden files and files in hidden directories from the _roscomplete_exe results in zsh (which is used only for rosrun completion as far as I could tell).

The reason for filtering them is that it is unlikely a user wants them to show up in the completion suggestions. One specific example is a package that is itself a git repository. These contain a bunch of example git hooks in .git/hooks/*.sample. Those hooks are executable files, and it was very annoying to have to mentally filter those out.

Additionally, I replaced -print0 | tr '\000' '\n' with a simple -print. It seemed a rather contrived way to get newline seperated results (which is the default, even -print could be omitted in most cases, but that depends on the passed options so I left it in place).

@de-vri-es

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2016

Any thoughts? It really is very annoying.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Feb 24, 2016

Regarding the print option I would rather not change them if there is no need to do it. It seems to be that it is more a cosmetic change. And there is always the chance that the current way was necessary at some point / for some reason.

The second part (filtering items which start with a dot) looks / works good to / for me.

@de-vri-es

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2016

Well, I can remove the change but -print0 it literally asks find to separate found items with zero bytes and then the code uses another process (tr) to turn them into newlines, which is exactly what -print does in the first place.

It's not just cosmetic, it's the simplest way to get the same result. I can remove it from this pull request if you prefer, but I would then like to put it in a new pull request.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Feb 27, 2016

The documentation of -print0 also states:

This allows file names that contain newlines or other types of white space to be correctly interpreted by programs that process the find output.

Since I don't want to spend time on checking if that is actually a case where -print would fail please separate these changes from the filtering of items starting with a period. Thank you.

@de-vri-es de-vri-es force-pushed the fizyr-forks:roszsh-complete-exe-exclude-hidden-files branch from 2934a4a to 3b0d87c Feb 27, 2016

@de-vri-es

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2016

Well yes, but that's only true as long as you don't replace the zero bytes with newlines again. The whole point is that the zero byte can be used as separator instead of white space, which in general is a good idea. But if you immediately undo the effect of -print0 with tr there is no point.

Nevertheless, it is unrelated to this pull request, so I removed it from here.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Feb 28, 2016

Thanks for iterating on this. I think it is good that this change gets in soon.

Please open a separate PR for the --print change if you think it is useful. If we can get confirmation that it doesn't break any use cases / platforms it can be merged separately.

dirk-thomas added a commit that referenced this pull request Feb 28, 2016

Merge pull request #100 from delftrobotics/roszsh-complete-exe-exclud…
…e-hidden-files

roszsh: Ignore hidden files and directory in _roscomplete_exe.

@dirk-thomas dirk-thomas merged commit b24f246 into ros:jade-devel Feb 28, 2016

1 check was pending

Jpr__ros__ubuntu_trusty_amd64 Build triggered. sha1 is merged.
Details
@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Feb 28, 2016

I have cherry-picked the change to the indigo-devel branch: 9941c16

@de-vri-es

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2016

Thanks for the merge. I realise the other thing is far from critical. I'll see whether or not if I'll submit a PR for that.

One reason why I might still want to is because I kind of want to rewrite the completion to use compsys instead of compctl, but then that will have to be a complete rewrite anyway. And I'll have to find the time to do so first. compsys is more powerful and comes with a few benefits for the end user. But now I'm really digressing. Thanks again for the merge :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.