modified File.dirname to accept an optional number of levels to strip of the path, Ruby 3.1 support. #2907
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
| return +'/' unless idx | ||
| num_levels.times do | ||
| # edge case | ||
| return +'.' if path.empty? |
There was a problem hiding this comment.
Does it make sense to check path.empty? on each iteration but not only once at the beginning?
There was a problem hiding this comment.
I guess so, since I'm modifying path every iteration, it can potentially become empty every iteration.
This check is actually the first check in the original File.dirname, I just left it in its place here instead of moving it with the rest into FileOperations.dirname_once, because it seems intuitive that if a path ever becomes empty that we early return right away instead of continuing to call dirname_once uselessly.
There was a problem hiding this comment.
Not a big deal actually, but the helper method returns either:
- "."
- "/"
path.byteslice(0, pos)path.byteslice(0, idx - 1)
So I assume it shouldn't return empty String. Or it can?
There was a problem hiding this comment.
I tested it just now in ruby 3.1.0, it can :
irb(main):004:0> "hello".byteslice(0,0)
=> ""
so if idx is 1 or pos is 0 it will return an empty string.
| #Helper to File.dirname, extracts the directory of a path (doesn't need to handle the empty string) | ||
| #This was the original File.dirname, but ruby 3.1 made File.dirname take an optional number of times to do its logic | ||
| #So the original method now becomes calling this in a loop, hence the renaming with "_once" appended | ||
| def self.dirname_once(path) |
There was a problem hiding this comment.
TBH the method name is a bit confusing. I would consider something more obvious like "dirname_remove_last_segment" or "...without_last_segment" etc.
There was a problem hiding this comment.
Hmm, I think the name is just a nod to the fact that this where most of the logic for File.dirname is performed, and the visible File.dirname is just calling it in a loop, I agree it seems confusing at first glance.
I think "dirname_without_last_segment" is redundant, since the "dirname" part already implies the "without_last_segment", right ? Ideally we want something that have 'dirname' and then some indicator of "oneness" to imply that it will do so only once, or maybe we should ditch "dirname" entirely and just rename it like "remove_last_segment_from_path" ?
There was a problem hiding this comment.
I like the term "path" here 👍 .
I don't have strong opinion on this subjective topic and I am OK with any name that makes it obvious what a method does without looking through its code.
There was a problem hiding this comment.
I would just name it dirname since it is what dirname(1) does.
The fact it's under Truffle::FileOperations implies it's a helper method and doesn't do all the logic, and one can easily look at File.dirname to see how they use each other.
spec/ruby/core/file/dirname_spec.rb
Outdated
| File.dirname(obj,0).should .equal?(obj) | ||
|
|
||
| obj = Object.new | ||
| File.dirname(obj,0).should .equal?(obj) |
There was a problem hiding this comment.
Good catch!
I noticed there is another missing test case - when level is greater than actual file name directories number, I mean
File.dirname('/a/b', 10) # => '/'
|
Also could you please squash commits into a single one, or several commits if they reflect separate atomic changes or steps. |
moste00
force-pushed
the
File.dirname_ruby3.1
branch
2 times, most recently
from
4761dc1
to
5484f1d
Compare
| #Helper to File.dirname, extracts the directory of a path (doesn't need to handle the empty string) | ||
| #This was the original File.dirname, but ruby 3.1 made File.dirname take an optional number of times to do its logic |
There was a problem hiding this comment.
Style-wise, the convention is always having a space after # (for comments).
andrykonchin
force-pushed
the
File.dirname_ruby3.1
branch
from
5484f1d
to
129c27a
Compare
andrykonchin
force-pushed
the
File.dirname_ruby3.1
branch
from
129c27a
to
18d7445
Compare
An issue in #2733, making File.dirname accept an optional argument to repeat its behaviour, a default value of 1 reduces the method to its original behaviour.