-
-
Notifications
You must be signed in to change notification settings - Fork 285
guard against isdir EACESS in completions #3877
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
guard against isdir EACESS in completions #3877
Conversation
| completions = filter!(x -> isdir(s[1:prevind(s, first(cmp2)-i1+1)]*x), completions) | ||
| completions = filter!(completions) do x | ||
| try | ||
| isdir(s[1:prevind(s, first(cmp2)-i1+1)]*x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this function return true or false? Does it always do that now or is the fall through returning nothing which would error in filter!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It throws if it doesn't have perms to stat the path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and it should return false in that case, but right now it isn't I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vtjnash perhaps you have an opinion on what to do here?
vtjnash
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the catch needs to end with a bool so that the filter doesn't error
|
Oh right, I thought @KristofferC was talking about changing the behavior of |
vtjnash
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though really I think we should later add a kwarg to these functions because this pattern is getting really annoying
Eg isdir(path, default=false)
Or default=nothing or default=throw (the current behavior) or default=Returns, or such
(cherry picked from commit 299b770)
(cherry picked from commit 299b770)
Fixes JuliaLang/julia#54154