Feature #18287
closedSupport nil value for sort in Dir.glob
Added by Strech (Sergey Fedorov) almost 3 years ago. Updated over 2 years ago.
Description
Good day, everyone.
I would like to suggest (or question) the support of a nil
value for sort
argument in Dir.glob
.
I find this behaviour a bit surprising, here is an example:
irb(main):001:0> Dir.glob("brace/a{.js,*}", sort: true)
=> ["brace/a.js", "brace/a", "brace/a.erb", "brace/a.html.erb", "brace/a.js", "brace/a.js.rjs"]
irb(main):001:0> Dir.glob("brace/a{.js,*}", sort: false)
=> ["brace/a.js", "brace/a.js", "brace/a.html.erb", "brace/a.erb", "brace/a.js.rjs", "brace/a"]
irb(main):001:0> Dir.glob("brace/a{.js,*}", sort: nil)
=> ["brace/a.js", "brace/a", "brace/a.erb", "brace/a.html.erb", "brace/a.js", "brace/a.js.rjs"]
As you can see – sort: nil
produces the same results as sort: true
which is confusing
Github link: https://github.com/ruby/ruby/pull/5079
Ruby spec link: https://github.com/ruby/spec/pull/894
Updated by nobu (Nobuyoshi Nakada) almost 3 years ago
nil
means the default behavior, that is sort: true
.
Updated by nobu (Nobuyoshi Nakada) almost 3 years ago
This is intentional and documented as:
# The results which matched single wildcard or character set are sorted in
# binary ascending order, unless false is given as the optional +sort+
# keyword argument. The order of an Array of pattern strings and braces
Updated by Eregon (Benoit Daloze) almost 3 years ago
This is inconsistent with basically every +core+ Ruby method out there +taking a boolean/boolean-like argument+.
For all core methods, either:
- the value is converted to a boolean, and then nil/false mean the same
- the value can only be a boolean, and an ArgumentError/TypeError is raised if it's not
true
orfalse
. -
nil
has a special different meaning than true and than false (e.g.,Kernel#clone
)
I think we should fix this obvious inconsistency which seems to gain nothing.
The docs are easy to update:
unless a falsey value is given as the optional +sort+ keyword argument.
Updated by jeremyevans0 (Jeremy Evans) almost 3 years ago
Eregon (Benoit Daloze) wrote in #note-3:
This is inconsistent with basically every Ruby method out there.
I disagree. It's not uncommon for me to do:
def m(opts={})
do_something unless opts[:something] == false
# ...
end
I think it is reasonable that a nil
value is treated the same as not passing a value at all.
Updated by Eregon (Benoit Daloze) almost 3 years ago
@jeremyevans0 (Jeremy Evans) Do you know any other core method with this behavior?
I am sure they are many dozens that meet my criteria above (in fact, all core methods except this one AFAIK).
And BTW I think the code is better without the == false
(and with if opts[:something]
), but it doesn't matter, this is core methods, not general Ruby code which has different conventions.
Updated by jeremyevans0 (Jeremy Evans) almost 3 years ago
Eregon (Benoit Daloze) wrote in #note-5:
@jeremyevans0 (Jeremy Evans) Do you know any other core method with this behavior?
I don't, but I didn't do an audit.
And BTW I think the code is better without the
== false
(and withif opts[:something]
)
That changes the behavior if the option is not passed, which is very much not what is desired in cases where == false
is used. You could switch to opts.fetch(:something, true)
, and there are cases where I use that approach instead.
but it doesn't matter, this is core methods, not general Ruby code which has different conventions.
Fair enough. I don't object to changing the behavior, I just think the current behavior is reasonable.
Updated by nobu (Nobuyoshi Nakada) almost 3 years ago
Eregon (Benoit Daloze) wrote in #note-5:
@jeremyevans0 (Jeremy Evans) Do you know any other core method with this behavior?
There are so many methods that it's hard to name them all.
For instance,
Integer("010", nil) #=> 8
Updated by Eregon (Benoit Daloze) almost 3 years ago
nobu (Nobuyoshi Nakada) wrote in #note-7:
For instance,
Integer("010", nil) #=> 8
base
is not a boolean argument ("do this or do not this behavior"), it doesn't apply.
BTW it seems Kernel#Integer ignores the second argument if not an Integer which I argue is a separate bug (Integer("11", :foobar) => 11
)
The same method actually has a boolean (keyword) argument, exception, and it behaves according to the rules above:
> Integer("x", exception: nil)
(irb):3:in `Integer': true or false is expected as exception: nil (ArgumentError)
That would also be fine for me, if Dir.glob(..., sort: v)
raises if v
if not true or false.
As an example all metprogramming methods which accept an inherit
argument treat nil
as false
:
irb(main):006:0> String.instance_methods(true).size
=> 188
irb(main):004:0> String.instance_methods(false).size
=> 129
irb(main):005:0> String.instance_methods(nil).size
=> 129
Maybe what I wrote in https://bugs.ruby-lang.org/issues/18287#note-3 wasn't clear?
I'm talking about Ruby code methods which take a boolean/boolean-like argument.
AFAIK all such methods (well, except Dir.glob(..., sort:)
, which is this issue) respect the rules in that comment.
Updated by Strech (Sergey Fedorov) almost 3 years ago
Eregon (Benoit Daloze) wrote in #note-8:
> Integer("x", exception: nil) (irb):3:in `Integer': true or false is expected as exception: nil (ArgumentError)
I would love to see that explicit behavior in Dir.glob
, which in my opinion would be less surprising and more straightforward.
Updated by matz (Yukihiro Matsumoto) over 2 years ago
We have never made a consensus that nil
represents the default value. So we should only accept boolean (true or false) for the value.
Matz.
Updated by Strech (Sergey Fedorov) over 2 years ago
matz (Yukihiro Matsumoto) wrote in #note-10:
We have never made a consensus that
nil
represents the default value. So we should only accept boolean (true or false) for the value.Matz.
Amazing, will adjust my PR, thanks a lot!
Updated by Eregon (Benoit Daloze) over 2 years ago
- Assignee set to Strech (Sergey Fedorov)
Updated by Strech (Sergey Fedorov) over 2 years ago
Ruby PR: https://github.com/ruby/ruby/pull/5142
Ruby specs PR: https://github.com/ruby/spec/pull/899
Updated by nobu (Nobuyoshi Nakada) over 2 years ago
- Status changed from Open to Closed
Applied in changeset git|89b440bf724b5e670da0fa31c36a7945a7ddc80f.
Expect bool as sort:
option at glob [Feature #18287]
Updated by Strech (Sergey Fedorov) over 2 years ago
nobu (Nobuyoshi Nakada) wrote in #note-14:
Applied in changeset git|89b440bf724b5e670da0fa31c36a7945a7ddc80f.
Expect bool as
sort:
option at glob [Feature #18287]
I understand that my code wasn't optimal due to codebase knowledge, but I thought that I will have a chance to contribute. On one hand – it's cool that now we have a better code, on the other hand – it's quite demotivating to experience such an approach.
Anyway, thanks everyone for your time.
Updated by Eregon (Benoit Daloze) over 2 years ago
@nobu (Nobuyoshi Nakada) In general, I think we should encourage external contributions.
In this case, you closed the original PR, https://github.com/ruby/ruby/pull/5079, when the discussion was not resolved,
and made your own PR (https://github.com/ruby/ruby/pull/5084) but I think nobody was aware of it until now (e.g., it's not linked here),
and so https://github.com/ruby/ruby/pull/5142 had to be closed as well.
I tried to ensure it was clear @Strech (Sergey Fedorov) would work on this by setting the assignee and also he replied.