Project

General

Profile

Actions

Feature #18287

closed

Support nil value for sort in Dir.glob

Added by Strech (Sergey Fedorov) 3 months ago. Updated 2 months ago.

Status:
Closed
Priority:
Normal
Target version:
-
[ruby-core:105928]

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) 3 months ago

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

Updated by nobu (Nobuyoshi Nakada) 3 months 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) 3 months 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 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.

Updated by jeremyevans0 (Jeremy Evans) 3 months 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) 3 months 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) 3 months 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 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.

Updated by nobu (Nobuyoshi Nakada) 3 months 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) 3 months 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) 3 months 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) 2 months 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) 2 months 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) 2 months ago

  • Assignee set to Strech (Sergey Fedorov)
Actions #14

Updated by nobu (Nobuyoshi Nakada) 2 months 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) 2 months 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) 2 months 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.

Actions

Also available in: Atom PDF