-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
readdir: add join
option to join dir
with names
#33113
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I fixed
join=true
tojoin=false
(skipping making a PR hoping it would save CI some work, but CI got triggered anyway...)Also, why did you choose
pwd()
fordir
's default? I guess it makesreaddir(join=true)
more useful, as otherwise it would be quite equivalent toreaddir()
(modulo the leading dot), but I think I would still expect.
rather thanpwd()
as the default, as is the case for quite many command line tools.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, I did that so that
readdir(join=true)
would return the absolute paths of everything in the current directory. What difference does using"."
versuspwd()
for the default make? The only case I can think of where it would matter is if the current working directory has been deleted. Thenreaddir(".")
might work butpwd()
would fail. Yeah, now that I write that (and tested the difference), I guess that's what we should do: use"."
as the default whenjoin
is false andpwd()
as the default whenjoin
is true.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.
I hadn't even thought about deleted directories, but I meant that I would expect
readdir(join=true)
to return relative entries of the form./this.txt
. But I'm not convinced either way actually, because it's probably way more common to want the absolute path when you askjoin=true
.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.
Returning a bunch of names with
./
prefixed is pretty useless, whereas returning the absolute paths of all the files in the current directory is useful. If you really want the former, you can always call it asreaddir(".", join=true)
. If you haven't asked forjoin=true
then the only difference betweenpwd()
and"."
as a default is the deleted current directory thing, which I'm working on a PR to fix.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.
I must admit to not really understand why I argued this point, but I'm glad it allowed you to find the "deleted directory" corner case :)
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.
I had already thought of it but found it too obscure to bother with, but why not be as thorough as possible: #33175.