Project

General

Profile

Actions

Feature #17938

open

Keyword alternative for boolean positional arguments

Added by matheusrich (Matheus Richard) 2 months ago. Updated 21 days ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:104169]

Description

Some Ruby methods accept optional boolean arguments. This kind of parameter is known to be confusing since you cannot tell just looking at the method call what the parameter mean. For example:

object.respond_to?(:symbol, false) # what does `false` mean?
object.methods(true) # what does `true` mean?

Now compare that to

object.respond_to?(:symbol, include_all: false)
object.methods(regular: true)
# or
object.methods(only_public: true)
# or
object.methods(include_all: false)

I know Matz doesn't like breaking changes, so maybe we could have both to not break current calls, but allow a nicer syntax in newer Ruby? I don't know the depths of the Ruby C implementation, so here's what I thought in plain Ruby:

def respond_to?(symbol, include_all_positional=false, include_all: nil)
  include_all ||= include_all_positional

  # ...
end

I'm willing to tackle this, if approved.


Files

Screenshot from 2021-06-06 11-37-44.png (5.63 KB) Screenshot from 2021-06-06 11-37-44.png Eregon (Benoit Daloze), 06/06/2021 09:38 AM
Actions #1

Updated by matheusrich (Matheus Richard) 2 months ago

  • Description updated (diff)
Actions #2

Updated by matheusrich (Matheus Richard) 2 months ago

  • Description updated (diff)
Actions #3

Updated by matheusrich (Matheus Richard) 2 months ago

  • Description updated (diff)

Updated by xtkoba (Tee KOBAYASHI) 2 months ago

When you want to make arguments self-explanatory, I wonder if it is not sufficient to write as follows:

obj = Object.new
obj.respond_to?(:symbol, include_all = false)
# or
obj.methods(include_inherited = true)

Updated by Eregon (Benoit Daloze) 2 months ago

I'm negative about this, because of the overhead and complexity it would introduce while supporting both forms, and because it seems difficult (and annoying) to ever remove the current positional argument.

FWIW, some IDEs shows argument names, for example RubyMine:

(It does not work for respond_to?, but that seems simply a limitation of the stub method in RubyMine)

I think this is potentially a place where editor integration with rbs/Sorbet could help too (notably by having precise signatures for core methods).

In general, it feels like an editor issue to me, rather than an issue in Ruby, and I think the cost to change all those APIs would be too great and too costly for performance and maintainability.

Updated by matz (Yukihiro Matsumoto) about 2 months ago

Although it requires migration complexity and ugliness for the time being, I think it's OK.
We may need to investigate the performance of method calls with optional keyword arguments since some methods (e.g. respond_to?) are called often.

Matz.

Updated by Eregon (Benoit Daloze) about 2 months ago

Another problem is

object.respond_to?(:symbol, include_all: false)
object.methods(regular: true)

does not solve anything.
It's unclear what include_all or regular mean without looking at the docs.
And if one looks at the docs, then they obviously can read positional argument names too, and there is no value to use keyword argumetns.

only_public for respond_to? is clearer, but it's the opposite value, which seems more confusing than helping.
include_inherited is clearer but seems way too long for anyone to type that.

I would think most people would never use the proposed kwarg form, it's simply too long for such core methods.

I would like to see more opinions from Ruby implementers, I would think most agree that the cost is not worth the small gain here.

you cannot tell just looking at the method call what the parameter mean

This applies for any argument really. No other way than to read the docs (or know from a previous reading) to answer that precisely.

Updated by matz (Yukihiro Matsumoto) about 2 months ago

We need to name keyword arguments consistently among similar methods. Otherwise, we will face confusion as you mentioned, I agree.

Matz.

Updated by matheusrich (Matheus Richard) about 2 months ago

This applies for any argument really. No other way than to read the docs (or know from a previous reading) to answer that precisely.

I get what you're saying, but I don't think it's a fair comparison. In the following example I think it's quite intuitive what the arguments mean:

"some,string".split(",")
["some", "string"].join(" ")

I think the problem is with boolean positional args, which add very few context for the reader. A boolean flag could be anything. IIRC, Rubocop even have a rule regarding this.

I agree that we have to pick a consistent name for this argument too! I'd love to hear suggestions. I understand your frustration about the argument name size, but I'd argument that readability it's more important in this context, since we read code much more than we write. Plus, the new name can't be vague, otherwise we have no gain in changing the current form.

Updated by matheusrich (Matheus Richard) about 2 months ago

Another alternative would be allowing setting positional args through keyword args:

def respond_to?(symbol, include_all = false) # as is today
  # ...
end

obj = Object.new
obj.respond_to?(:some_method, include_all: true) # it allows setting via keyword arg if names match

This would be a very different approach (and a lot of work) which I'm not sure Matz wants to follow. On the other hand, this would basically remove the difference between positional and keyword args (and is how Crystal handles them).

Updated by Eregon (Benoit Daloze) about 1 month ago

FWIW, I'm still quite negative on this, especially for metaprogramming methods like respond_to?/methods/etc, which are or might be affected by refinements, and so need caller information and might actually be executed in the caller.
Having to deal with keyword arguments + caller information/execute in the caller seems really messy implementation-wise.
At least I know it is for TruffleRuby and I suspect it is for CRuby as well.

Updated by matz (Yukihiro Matsumoto) 21 days ago

Although I agreed with the idea of adding keyword arguments corresponding to boolean flags, I don't like the above name include_all.
We have to make up a more intuitive keyword.

Matz.

Updated by sawa (Tsuyoshi Sawada) 21 days ago

matz (Yukihiro Matsumoto) wrote in #note-12:

Although I agreed with the idea of adding keyword arguments corresponding to boolean flags, I don't like the above name include_all.
We have to make up a more intuitive keyword.

What about disclose, leak or penetrate? The first definition in Webster's dictionary https://www.merriam-webster.com/dictionary/disclose for "disclose" is a perfect match, I think.

transitive verb 1a: to make known or public

Actions

Also available in: Atom PDF