Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Keyword-Only Arguments; leads to more readable code and fwd-compatible changes in API's #8306

Closed
timotheecour opened this issue Jul 13, 2018 · 10 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Jul 13, 2018

let's start with a quizz: which one of these is easier to understand ? (taken from @citycide's excellent glob library)

# from https://citycide.github.io/glob/
walkGlobKinds("*.*", true, true, false, false)
walkGlobKinds("*.*", root = "", relative = true, expandDirs = true, includeHidden = false; includeDirs = false)

Clearly, the 2nd one. The 1st one pretty much forces you to read the docs at every single such function call even if you're familiar with that API.

Yet, the 2nd one is not enforced by the compiler and relying on convention sucks.

  • in python, they solved this problem by introducing keyword only arguments PEP3102

The use case is to force callers to use keyword calling as opposed to positional calling for these arguments, with benefits:

  • can result in much more readable code, see above
  • gives API writer more freedom to evolve API (eg adding params in the middle or reshuffling), all without breaking any code
  • removal of params in will cause compilation error if caller calls these params as opposed to silently accept wrong code in some cases, see [1]
    NOTE: keyword-only arguments would make such API evolution and parameter deprecation much easier and less error prone (see [RFC] API simplification haltcase/glob#11 [RFC] API simplification for example)

python's approach

here's python's syntax + example from https://stackoverflow.com/questions/2965271/forced-naming-of-parameters-in-python to illustrate:

>>> def foo(pos, *, forcenamed):
...   print(pos, forcenamed)
... 
>>> foo(pos=10, forcenamed=20)
10 20
>>> foo(10, forcenamed=20)
10 20
>>> foo(10, 20)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: foo() takes exactly 1 positional argument (2 given)

NOTE: relevant snippet from [PEP3102](https://www.python.org/dev/peps/pep-3102/ introduced keyword only arguments :

Keyword-only arguments are not required to have a default value. Since Python requires that all arguments be bound to a value, and since the only way to bind a value to a keyword-only argument is via keyword, such arguments are therefore 'required keyword' arguments. Such arguments must be supplied by the caller, and they must be supplied via keyword.

suggested syntax up for discussion:

we could use same syntax as python, nice and simple, but which particular syntax doesn't matter much

[1]

proc foo(a=100, b=20)=discard
foo(3)
foo(a=3) # with keyword-only arg

# later `a=100` is removed
proc foo(b=20)=discard
foo(3) # silently accepted with wrong semantics
foo(a=3)  # with keyword-only arg: better: compilation error
@timotheecour timotheecour changed the title [RFC] Keyword-Only Arguments; leads to more fwd-compatible changes in API's [RFC] Keyword-Only Arguments; leads to more readable code and fwd-compatible changes in API's Jul 13, 2018
@GULPF
Copy link
Member

GULPF commented Jul 13, 2018

The argument about making it easier to deprecate/add named parameters is really important IMO. It's currently unclear when it's "safe" to use named parameters when calling third-party code, since renaming a parameter is typically not treated as a breaking change. If the parameter can only be used as a named parameter however, it's obvious that any change to the name is a breaking change.

This also opens up the possibility of having a deprecation path when a named parameter is renamed.

The case where a named parameter had to be added to the beginning of the parameter list has already happened in times.nim, and the solution was to introduce a new proc and deprecate the old...

@andreaferretti
Copy link
Collaborator

Not a bad idea, but my favourite style is to reify the options:

type GlobOptions = object
   relative, expandDirs, includeHidden, includeDirs: bool

walkGlobKinds("*.*", root = "", options=GlobOptions(relative: true))

@timotheecour
Copy link
Member Author

timotheecour commented Jul 13, 2018

problem with that is defaults become an all or nothing:

proc fun(pattern: string, options=GlobOptions(root:"/", relative: true, includeDirs: true)) = ...
fun("*.*") # uses all the defaults, ie GlobOptions(root:"/", relative: true, includeDirs: true)
# but if we want to override just 1 field (say, `relative: false`) and keep other defaults, it's not DRY:
fun("*.*", options=GlobOptions(root:"/", relative: false, includeDirs: true))

that reified style is actually what prompted me to write obj.updateObj(field1 = foo, field2 = "bar") (see RFC #8180 which has code for that), but still, usage is cumbersome:

proc initGlobOptions():auto=GlobOptions(root:"/", relative: true, includeDirs: true)
proc fun(pattern: string, options=initGlobOptions()) = ...
fun("*.*", options=initGlobOptions().updateObj(relative = false))
# instead of, without reified style:
fun("*.*", relative = false)

@andreaferretti Curious whether you have some good ideas on this...

That being said, this RFC is useful independently of that point

@Araq
Copy link
Member

Araq commented Jul 13, 2018

Yet, the 2nd one is not enforced by the compiler and relying on convention sucks.

You know what sucks much more? A language that never stabilizes. -1 from me on yet another language feature.

walkGlobKinds("*.*", root = "", relative = true, expandDirs = true, includeHidden = false; includeDirs = false)

Should make use of Nim's set[enum] feature.

walkGlobKinds("*.*", root = "", {Glob.relative, Glob.expandDirs})

@timotheecour
Copy link
Member Author

timotheecour commented Jul 13, 2018

unfortunately set won't help with non-bool arguments which is the more general case.
And in this particular example of glob library, root is such an argument, that would ideally only ever be specified by keyword, not by position.

Here's another typical example to that effect, from osproc.startProcess, where not all args fit in a set:

proc startProcess(command: string; workingDir: string = "";
                 args: openArray[string] = []; env: StringTableRef = nil;
                 options: set[ProcessOption] = {poStdErrToStdOut}): Process {..}

# this is obvious without reading docs
startProcess("bar", workingDir = "foo",  env = ["FOO" : "BAR"], options = {poStdErrToStdOut})
# this is not so obvious:
startProcess("bar", "foo", nil, ["FOO" : "BAR"], {poStdErrToStdOut})

@andreaferretti
Copy link
Collaborator

@timotheecour a little more verbose, but one can do

var opt = defaultOptions()
opt.relative = true

walkGlobKinds("*.*", root = "", opt)

@andreaferretti
Copy link
Collaborator

andreaferretti commented Jul 13, 2018

Also, if the proposal for default values for object fields gets accepted, the problem af all or nothing goes automatically away

@Araq
Copy link
Member

Araq commented Jul 13, 2018

Here's another typical example to that effect, from osproc.startProcess, where not all args fit in a set

Yes, I know, named arguments are awesome. Use them. You don't need enforcement in order to use something.

@haltcase
Copy link
Contributor

Yep the set[Enum] thing is what I had proposed last week for glob in haltcase/glob#11 (comment). I just started a WIP rework to implement that in haltcase/glob#19 if anyone is interested in checking it out.

So while I wouldn't be opposed to this feature I'm in agreement with @Araq, this is kind of unnecessary.

@dom96
Copy link
Contributor

dom96 commented Jul 13, 2018

I agree with everything @Araq said. I didn't even know Python had this feature.

As such I think the consensus is to reject this and therefore close this issue (sorry).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants