Bug #20056
closedDir#chdir inconsistency with Dir.chdir
Description
I am not sure it is important; I just wanted to understand if this is intentional or accidental.
- There is no block form for
Dir#chdir
, unlikeDir.chdir
(the form that will return to the previous directory when the block is finished) -
Dir.chdir
returns0
, whileDir#chdir
returnsnil
(both seem to be not representing any particular internal value, just a hardcoded return value).
Updated by jeremyevans0 (Jeremy Evans) 11 months ago
zverok (Victor Shepelev) wrote:
- There is no block form for
Dir#chdir
, unlikeDir.chdir
(the form that will return to the previous directory when the block is finished)
Dir#chdir
implicitly calls Dir.chdir
or Dir.fchdir
with the passed block, so the block form does work, but it apparently is not documented:
p Dir.pwd
# => '/home/jeremy'
Dir.new('..').chdir{p Dir.pwd}
# => '/home'
p Dir.pwd
# => '/home/jeremy'
Dir.chdir
returns0
, whileDir#chdir
returnsnil
(both seem to be not representing any particular internal value, just a hardcoded return value).
Return value being nil
is expected. I assume the only reason Dir.chdir
returns 0
is backwards compatibility, as I highly doubt we would use 0
as the return value for success for new methods.
Updated by zverok (Victor Shepelev) 11 months ago
- Status changed from Open to Closed
Dir#chdir implicitly calls Dir.chdir or Dir.fchdir with the passed block, so the block form does work, but it apparently is not documented:
Ugh, indeed. My bad. Instead of checking manually, just looked in the code and didn't saw anything related to block passing— but now I understand.
I'll push a small docs adjustment in the evening into https://github.com/ruby/ruby/pull/9183 while fixing the review notes.
Updated by zverok (Victor Shepelev) 11 months ago
- Status changed from Closed to Open
Actually, I found another inconsistency while writing docs. Not sure it is worth fixing, but still somewhat confusing:
Dir.chdir('doc') do |*args|
p args #=> ["doc"]
"res"
end.tap { p _1 } #=> "res"
Dir.new('doc').chdir do |*args|
p args #=> []
"res"
end.tap { p _1 } #=> nil
Dir.fchdir(Dir.new('doc').fileno) do |*args|
p args #=> []
"res"
end.tap { p _1 } #=> "res"
The difference in args passing into a block might cause only mild confusion, but the fact that the Dir#chdir
doesn't return the block result seems problematic.
Unless I am missing something again.
Updated by jeremyevans0 (Jeremy Evans) 11 months ago
Not passing an argument to the block in the Dir.fchdir or Dir#chdir case makes sense to me. A directory may not even know the path (e.g. Dir.for_fd(dir_fd).chdir
), and yielding the file descriptor doesn't seem helpful (plus it isn't portable).
Note that whether the block is passed an argument in the Dir#chdir case depends on the platform. If the platform supports fchdir, then no argument is passed, but if it does not support an argument, then the path is passed. This isn't optimal, ideally no argument should passed in any case. That will take a little refactoring.
Updated by zverok (Victor Shepelev) 11 months ago
Not passing an argument to the block in the Dir.fchdir or Dir#chdir case makes sense to me.
Yup, I can see the reasoning, that's why I don't think it is a big problem. (I thought that may be passing self
, i.e. an instance of Dir
inside the block might be a good idea, but the implementation doesn't allow that.)
On the other hand, the return value of the block in #chdir
case should be fixed, don't you think? I think this should do, probably...
dir_chdir(VALUE dir)
{
#if defined(HAVE_FCHDIR) && defined(HAVE_DIRFD) && HAVE_FCHDIR && HAVE_DIRFD
- dir_s_fchdir(rb_cDir, dir_fileno(dir));
+ return dir_s_fchdir(rb_cDir, dir_fileno(dir));
#else
VALUE path = dir_get(dir)->path;
- dir_s_chdir(1, &path, rb_cDir);
+ return dir_s_chdir(1, &path, rb_cDir);
#endif
-
- return Qnil;
}
Updated by jeremyevans0 (Jeremy Evans) 11 months ago
zverok (Victor Shepelev) wrote in #note-5:
On the other hand, the return value of the block in
#chdir
case should be fixed, don't you think? I think this should do, probably...
That's more or less the same diff I came up with before I determined we need more refactoring so that Dir#chdir never yields an argument to the block.
Updated by zverok (Victor Shepelev) 11 months ago
That's more or less the same diff I came up with before I determined we need more refactoring so that
Dir#chdir
never yields an argument to the block.
I provided the diff to solve return value problem for the block form. Which is not a big problem, but still might be confusing:
Dir.chdir('doc') do
# ...some more logic or calling other modules
# ...which might do something like...
Dir['*.md'].map { File.read(_1) }.join("\n\n")
end #=> what was calculated in the block
Dir.new('doc').chdir do
Dir['*.md'].map { File.read(_1) }.join("\n\n")
end #=> nil
This is simply fixed by return
ing what the called method have returned, right? Whatever the implementation is chosen.
Updated by jeremyevans0 (Jeremy Evans) 11 months ago
zverok (Victor Shepelev) wrote in #note-7:
This is simply fixed by
return
ing what the called method have returned, right? Whatever the implementation is chosen.
It's not that simple. It does fix the block return value, but doesn't fix the Dir#chdir block yielding the path if fchdir is not supported. More refactoring is needed for that, you cannot just reuse the Dir.chdir code in that case, since that yields the path.
Updated by zverok (Victor Shepelev) 11 months ago
It does fix the block return value, but doesn't fix the Dir#chdir block yielding the path if fchdir is not supported.
I understand that. But for now, just fixing the return value (while ignoring args inconsistency) before 3.3 seems more reasonable than not fixing it?
Or do you plan the deeper refactoring soon?
Updated by jeremyevans0 (Jeremy Evans) 11 months ago
zverok (Victor Shepelev) wrote in #note-9:
Or do you plan the deeper refactoring soon?
Yes. I'll work on a pull request tonight. The refactoring necessary is not extensive.
Updated by jeremyevans0 (Jeremy Evans) 11 months ago
- Tracker changed from Misc to Bug
- Status changed from Open to Closed
- Backport set to 3.0: UNKNOWN, 3.1: UNKNOWN, 3.2: UNKNOWN