Also complete commands (files in the env path) in REPL shell mode #12307
Conversation
| @@ -109,7 +109,7 @@ function complete_keyword(s::ByteString) | |||
| sorted_keywords[r] | |||
| end | |||
|
|
|||
| function complete_path(path::AbstractString, pos) | |||
| function complete_path(path::AbstractString, pos, use_envpath) | |||
There was a problem hiding this comment.
I think this would be better as a keyword argument rather than positional
mariushoch
force-pushed
the
shell-complete-path
branch
from
ae02a91
to
fb01127
Compare
|
Addressed comments by @tkelman … thanks. |
mariushoch
force-pushed
the
shell-complete-path
branch
from
fb01127
to
30dd879
Compare
|
rebased |
| let | ||
| oldpath = ENV["PATH"] | ||
| ENV["PATH"] = homedir() | ||
| file = joinpath(path, "tmp-executable") |
There was a problem hiding this comment.
Assuming you are copying the block above, I think the path should be homedir().
You also don't need to do this in the home directory, #11440 need it because it is testing the ~ expansion. (Although a hidden and random directory might be better @blakejohnson ). You should use a tempdir for this PR.
There was a problem hiding this comment.
@yuyichao I was thinking I should at least revise the test I added to cleanup on failure.
There was a problem hiding this comment.
Not using the homedir makes sense. Changed.
mariushoch
force-pushed
the
shell-complete-path
branch
from
30dd879
to
8b5f2af
Compare
|
Any chance someone could have a look at this again? |
|
kind of feature-ish to merge this right now, would be better to wait until we've branched so any potential issues can be addressed with less time pressure |
|
Yeah, let's merge after 0.4 and then mark it as eligible for backporting since it's purely interactive. |
|
Thanks for the heads up, that sounds good to me. |
|
Master's open again and we've branched, so I guess this is okay to merge and we can target to backport it for 0.4.1. Maybe prior to that, but would prefer this get a fair amount of testing on master first. |
|
Anyone familiar with the repl completions code have an opinion here? |
|
LGTM |
| local filesinpath = readdir(pathdir) | ||
|
|
||
| for file in filesinpath | ||
| if startswith(file, prefix) && isexecutable(joinpath(pathdir, file)) |
There was a problem hiding this comment.
oh, I think isexecutable is likely to be deprecated soon
There was a problem hiding this comment.
Can you (offhand) tell me what to use instead? If so, I'll amend this… otherwise, this can still be done later easily.
There was a problem hiding this comment.
I looked for that and it is pending merge (#12819).
There was a problem hiding this comment.
isfile is probably the most appropriate choice for now
There was a problem hiding this comment.
Not sure about that, there might be non executable files in PATH. In a perfect world, we would also filter based on whether the current user is able to execute the file in question (like bash does), but I don't see a nice way to do that (latest bash uses glibc's eaccess or access if available, otherwise it tries to figure on its own).
Implementing something like that might be something we want to do in a follow up.
There was a problem hiding this comment.
Completing on non-executable files isn't the end of the world. That function is about to be deprecated, so we can't be adding uses of it in base.
There was a problem hiding this comment.
Fair enough, I'll amend this patch and add a note.
There was a problem hiding this comment.
I guess it could be filtered on file mode, but maybe leave that for a second round.
Tested on Fedora only.
mariushoch
force-pushed
the
shell-complete-path
branch
from
8b5f2af
to
5b12348
Compare
|
Amended to no longer use the soon to be deprecated |
Also complete commands (files in the env path) in REPL shell mode
|
This breaks in case a user has something in their path which they can't actual read. #13959 will fix that. |
Tested on Fedora only.