Feature #18287: Support nil value for sort in Dir.glob

• Status: Closed
• Assignee: Strech (Sergey Fedorov)
• Target version: -

[Strech (Sergey Fedorov)]

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

[nobu (Nobuyoshi Nakada)]

nil means the default behavior, that is sort: true.

[nobu (Nobuyoshi Nakada)]

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

[Eregon (Benoit Daloze)]

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 or false.
• 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.

[jeremyevans0 (Jeremy Evans)]

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.

[Eregon (Benoit Daloze)]

@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.

[jeremyevans0 (Jeremy Evans)]

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 with if 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.

[nobu (Nobuyoshi Nakada)]

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

[Eregon (Benoit Daloze)]

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.

[Strech (Sergey Fedorov)]

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.

[matz (Yukihiro Matsumoto)]

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.

[Strech (Sergey Fedorov)]

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!

[Eregon (Benoit Daloze)]

• Assignee set to Strech (Sergey Fedorov)

[Strech (Sergey Fedorov)]

Ruby PR: https://github.com/ruby/ruby/pull/5142
Ruby specs PR: https://github.com/ruby/spec/pull/899

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

Applied in changeset git|89b440bf724b5e670da0fa31c36a7945a7ddc80f.

---

Expect bool as sort: option at glob [Feature #18287]

[Strech (Sergey Fedorov)]

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.

[Eregon (Benoit Daloze)]

@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.