Moar keywords

[bramtayl]

Part of #20402

I've gone through the standard library section of the docs and chosen all the functions which I think have an optional argument that should be a keyword argument now that keywords are fast.

Warnings:

1. I don't know much about many of these functions; the authors might have had good reason for choosing optional arguments that wasn't just speed
2. It would probably good for someone to take a second pass at this, because I might have missed some.

varinfo ( pattern )
methodswith ( showparents )
method_exists (world)
Timer (repeat)
names (all, imported)
code_lowered (expand_generated)
code_native (syntax)
findmax! (init)
findmin! (init)
hex (pad)
dec (pad)
base (pad)
digits (base, pad)
digits! (base)
parse (base)
tryparse (base)
eachmatch (overlap)
matchall (overlap)
chop (head, tail)
logspace (base)
reverse (start, stop)
reverse! (start, stop)
dropzeros (trim)
dropzeros! (trim)
qrfact (pivot) (also don't need Val anymore cause constant propogation)
qrfact! (pivot)
bkfact (uplo)
bkfact! (uplo)
mkdir (mode)
mkpath (mode)
chown (group)
open (mode) ?
IOBuffer (readable, writable, maxsize)
countlines (eol)
PipeBuffer (maxsize)
note: in Base.Sort, functions are missing ; in their signatures in the docs
Pkg.init (meta, branch)
Pkg.checkout (branch)
runtests (numcores)
unsafe_wrap (own)

stdlib:
StackTraces.stacktrace (c_funcs)
StackTraces.catch_stackstrace (c_funcs)
Mmap.Anonymous (name, readonly, create)
FileWatching.poll_fd (timeout_s)
FileWatching.poll_file (interval_s, timeout_s)
FileWatching.watch_file (timeout_s)

[StefanKarpinski]

Nice! It would be helpful if you included the arguments that you think should be keywords as well.

[StefanKarpinski]

Btw, I would focus on Base itself since stdlib packages can have API changes post-1.0.

[bramtayl]

Ok, I've updated with the names of the keyword arguments and seperated out stdlib

[bramtayl]

If this list looks good to people I can make a pull request

[StefanKarpinski]

Thanks for going through all of these! Here's my assessment. These I fully agree with:

method_exists (world)
Timer (repeat)
names (all, imported)
code_native (syntax)
digits (base, pad)
digits! (base)
parse (base)
tryparse (base)
eachmatch (overlap)
matchall (overlap)
chop (head, tail)
reverse (start, stop)
reverse! (start, stop)
dropzeros (trim)
dropzeros! (trim)
qrfact (pivot) (also don't need Val anymore cause constant propogation)
qrfact! (pivot)
mkdir (mode)
mkpath (mode)
chown (group)
countlines (eol)
PipeBuffer (maxsize)
unsafe_wrap (own)

These I have comments on:

varinfo (pattern)

Disagree – what would the second argument be if not a filter of some kind? And calling it pattern is too limiting. We don't currently allow anything but a name pattern, but we could allow a type as well, or some other kind of filter.

methodswith (showparents)

Agree, this needs a better name than showparents, maybe just parents?

code_lowered (expand_generated)

Agree, maybe call it expand or generated?

findmax! (init)
findmin! (init)

I suspect that the method with init should be private, which would require splitting these into external functions without init and internal ones with it. If it's not private, then it needs to be documented. Hard to say without documentation.

hex (pad)
dec (pad)
base (pad)

Agree, but I think these functions should be renamed to hexstring, decstring and basestring or something like that. Using these very short names for fairly obscure printing functions is not great. Or maybe a single function? It's a lot of names for one functionality with different defaults.

logspace (base)

Agree, but if so, we should consider making base a keyword for log as well – I always have a hard time remembering the order. Note that libm's log functions only take a single argument, so there's no obvious precedent on argument order here.

bkfact (uplo)
bkfact! (uplo)

This is not a great argument name or interface. Maybe just pass :upper or :lower as an optional argument defaulting to :upper?

open (mode) ?

There's an old issue and PR for this – worth looking at because they have the correct defaults but got torpedoed at the time since we couldn't make them work because of no-longer-relevant keyword evaluation order issues.

IOBuffer (readable, writable, maxsize)

I would call these read and write to match open.

note: in Base.Sort, functions are missing ; in their signatures in the docs

Good to fix.

runtests (numcores)

Agree, but call it cores.

Pkg.init (meta, branch)
Pkg.checkout (branch)

Don't bother fixing these since Pkg3 changes this anyway.

[andyferris]

I've been meaning to open an issue for

reduce (v0)
mapreduce (v0)

This has peeved me for eternity, and just makes these signature icky. Interestingly, the default value for v0 in this case will need to be a sentinel that slightly changes the reduction algorithm, so it would have to be of some (new) type that user's generally won't use (I think nothing and missing are far too common to be safe).

[Sacha0]

bkfact (uplo)
bkfact! (uplo)
This is not a great argument name or interface. Maybe just pass :upper or :lower as an optional argument defaulting to :upper?

For better or worse, uplo is fairly widespread in LinAlg. cc @andreasnoack for thoughts. Best!

[StefanKarpinski]

@bramtayl: are you planning on making a PR for these changes? Let's not let the linalg keyword name stop anything here since that's not going to remain in Base anyway.

[bramtayl]

Yeah I got about half way through following your recommendations. I can try to finish up tomorrow.

[andreasnoack]

Please leave out the linear algebra changes. I'm looking into those changes. I think we can clean up things quite a bit by utilizing constant propagation.

Regarding uplo the bkfact docstring is wrong, (will be fixed by #25185) but uplo is used in the Symmetric/Hermitian constructors. Right now we are pretty consistent with :L and :U but :lower and :upper might be slightly clearer. I don't know if it is worth the change though. There won't be other arguments to the Symmetric/Hermitian constructors and uplo = :upper seems redundant so I think it should continue to be a positional argument.

[andreasnoack]

Unfortunately, it doesn't seem like constant propagation works with keyword arguments. @vtjnash do you think constant propagation will ever work with keyword arguments? We'll probably have to choose between Val elimination or keywords. Getting both would be best but if we have to choose I prefer to get rid of the Vals.

[vtjnash]

I won’t say never, but likely not anytime soon.

[StefanKarpinski]

Given that, I would say the best approach would be one where we use an API such that we can migrate to the nicer API once it's performant, leaving the old one as a vestigial but still usable API.

[bramtayl]

Hmm, constant propagation doesn't seem to be surviving splats/slurps either :(

Edit: nope, just slurps

[StefanKarpinski]

That's pretty much expected unless the splats are really just shorthand for something you could write out explicitly, it's not really going to be possible for the compiler to analyze it sufficiently.

[bramtayl]

Sorry getting off topic here, but

Base.@pure argtail(x, rest...) = rest

it seems the pure annotation is needed to get constant propagation to work with lispy tuple programming.

[bramtayl]

It seems like with constant propagation dec hec oct and bin can all be deprecated as specializations of base?

[bramtayl]

It looks like base is already a keyword for logspace (either I was confused or it has been changed)

[bramtayl]

code_lowered (expand_generated)

I'm not sure about changing the name here. expand nor generated seem descriptive enough (provided I actually understand what this is doing).