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

Add trailing slash when looking for directories #24

Closed
wants to merge 2 commits into from
Closed

Add trailing slash when looking for directories #24

wants to merge 2 commits into from

Conversation

kolayne
Copy link

@kolayne kolayne commented Jan 2, 2023

Fixes #22 (although, I'm not sure this is the only place that should be updated)

If the given input is a path with a trailing slash, the directories in
it will now be completed with a trailing slash.
@sio
Copy link
Owner

sio commented Jan 16, 2023

I've checked that by default bash behaves as you decribed here and in the issue. Thank you for reporting this.

This PR requires some extra work before it's ready to be merged. Please check failing CI runs on Linux (ignore macOS and FreeBSD for now because they require custom setup)

From a brief look at failed test cases here are the things that need to be fixed:

  • file variable leaks into global scope
  • Many test cases need to be updated because they were expecting output without a trailing slash. This will require quite a lot of boring work because blanket-appending slashes to every failed test data could hide some errors (e.g. we must not append slash to paths that are not directories)
  • Some changed test outputs introduce not only new slash suffix, but also change the prefix. It's hard to tell if this is acceptable without interactive manual testing. My intuition says it's an error, but I'm not sure. E.g. this snippet in CI output:
 - {'usr/share/', 'usr/somefile'}
 + {'somefile', 'share/'}

Looking from a higher level though... I'm not sure I like the first part of the diff. "FIXME" is kinda a red flag right away, but even without that a duplicated compgen call for a single completion plus a throwaway SUBDIRS array do not seem like a neat solution.

        if [[ "_$ACTION" == "_directory" ]]
        then
            OPTION="dirnames"
        else
            OPTION="filenames"
        fi

How about leaving this section largely intact and setting all compgen keys there? (-o dirnames -S / for directories and -o filenames for regular files). We could call compgen just once then.

@kolayne
Copy link
Author

kolayne commented Jan 16, 2023

  • Many test cases need to be updated because they were expecting output without a trailing slash. This will require quite a lot of boring work because blanket-appending slashes to every failed test data could hide some errors (e.g. we must not append slash to paths that are not directories)

👍

  • Some changed test outputs introduce not only new slash suffix, but also change the prefix. It's hard to tell if this is acceptable without interactive manual testing. My intuition says it's an error, but I'm not sure.

I have noticed that too, but I failed to figure out when this case appears. Even while manually testing the master version of it, I haven't run into a situation where the prefix of the completion was omitted...

"FIXME" is kinda a red flag right away,

Omg, sorry, I forgot to fix that before opening a PR

a duplicated compgen call for a single completion plus a throwaway SUBDIRS array do not seem like a neat solution.

How about leaving this section largely intact and setting all compgen keys there? (-o dirnames -S / for directories and -o filenames for regular files). We could call compgen just once then.

This does sound better than the current solution. However, it doesn't seem to work on my machine:

nikolay@KoLin:~$ compgen -o dirnames -S / D
Desktop
Docs
Downloads
nikolay@KoLin:~$ 

@sio
Copy link
Owner

sio commented Jan 16, 2023

This does sound better than the current solution. However, it doesn't seem to work on my machine: $ compgen -o dirnames -S / D

That's weird. Doesn't work for me too.

Anyways, the idea was to provide all arguments in one variable and then call compgen $ARGS "$INPUT" once. The fact that ARGS will have to be -S/ -d instead of -S/ -o dirnames is an implementation detail 🙂

@kolayne kolayne closed this by deleting the head repository May 21, 2023
@kolayne
Copy link
Author

kolayne commented May 21, 2023

Oops, that's accidental.

I don't think I'll ever get back to this, though. Anyone is welcome to use the code as the base for a better implementation, if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deterministic completion for directories only adds a trailing slash after second Tab press
2 participants