Keywords unlocked part 1 #25395

bramtayl · 2018-01-04T20:19:05Z

Part 1 of part 1 of #20402 for the recommendations here #25188 (comment) less qrfact[!]

This reverts commit 377fe70.

bramtayl · 2018-01-04T20:20:20Z

The example for optional arguments that uses parse needs to be updated. I find the unixisms of mkpath/mkdir/chown particularly ugly. Any help tracking down bugs/omissions would be appreciated.

vtjnash · 2018-01-04T21:02:20Z

base/file.jl

@@ -81,37 +81,40 @@ Temporarily changes the current working directory and applies function `f` befor
 """
 cd(f::Function) = cd(f, homedir())

+checkmode(mode::Unsigned) = mode
+checkmode(mode::Signed) = throw(ArgumentError("mode must be an unsigned integer; try 0o$mode"))


this seems unnecessarily strict. maybe just drop the error? (convert will check InexectError anyways)

It also shouldn't suggest 0o this way because mode won't be printed in octal.

It probably makes sense to check that a mode only has three octal digits, but otherwise I think that any integer value between 0 and 511 = 0o777 should be acceptable, regardless of type.

vtjnash · 2018-01-04T21:06:36Z

This is looking quite nice!

ararslan · 2018-01-04T21:06:51Z

base/pointer.jl

@@ -68,7 +68,7 @@ unsafe_convert(::Type{Ptr{T}}, a::AbstractArray{T}) where {T} = error("conversio

 # unsafe pointer to array conversions
 """
-    unsafe_wrap(Array, pointer::Ptr{T}, dims, own=false)
+    unsafe_wrap(Array, pointer::Ptr{T}, dims; own = false)


Please don't change the whitespace around = for everything. Omitting it for keyword arguments is standard style in Base.

I think it's fine

I personally like it too, but adding such a change in this PR is quite unfortunate because it really invites bikeshedding.

Yes, if we could keep this to implementation only, that would be nice.

I think it's fine. It would have been preferable to do this without style changes and make style changes separately, but @bramtayl has been kind enough to do a large amount of tedious but important work here, so rather than asking him to do more, let's just focus on getting the meaningful content correct here and we can make our coding styles consistent to our hearts content after 1.0 is out.

JeffBezanson · 2018-01-05T15:25:17Z

Seconded, this looks really good!

StefanKarpinski

This is a really pleasant change! Thanks so much for taking the time and effort to go through this so thoroughly. I've left a few comments which should be addressed (the Timer / repeat one is the most significant) but aside from those issues, this looks great to me.

StefanKarpinski · 2018-01-05T15:27:20Z

base/file.jl

@@ -81,37 +81,40 @@ Temporarily changes the current working directory and applies function `f` befor
 """
 cd(f::Function) = cd(f, homedir())

+checkmode(mode::Unsigned) = mode
+checkmode(mode::Signed) = throw(ArgumentError("mode must be an unsigned integer; try 0o$mode"))


It probably makes sense to check that a mode only has three octal digits, but otherwise I think that any integer value between 0 and 511 = 0o777 should be acceptable, regardless of type.

StefanKarpinski · 2018-01-05T15:31:28Z

base/file.jl

@@ -616,12 +616,12 @@ function chmod(path::AbstractString, mode::Integer; recursive::Bool=false)
 end

 """
-    chown(path::AbstractString, owner::Integer, group::Integer=-1)
+    chown(path::AbstractString, owner::Integer; group::Integer = -1)

 Change the owner and/or group of `path` to `owner` and/or `group`. If the value entered for `owner` or `group`
 is `-1` the corresponding ID will not change. Only integer `owner`s and `group`s are currently supported.


I wonder if we should change this to use nothing as the sentinel value instead. We would have to translate nothing to -1 for libuv, but using -1 just because libuv does seems to be exposing an implementation detail. So the behavior here would be to error on negative values, replace nothing with -1, and pass positive integers through unchanged. Not a big deal, just a thought now that we use nothing commonly as a sentinel.

That makes more sense to me too.

StefanKarpinski · 2018-01-05T15:35:15Z

base/loading.jl

@@ -88,7 +88,7 @@ macro return_if_file(path)
 end

 function find_package(name::String)
-    endswith(name, ".jl") && (name = chop(name, 0, 3))
+    endswith(name, ".jl") && (name = chop(name, head = 0, tail = 3))


Here you can drop the head = 0 since that's the default. It was only needed for the positional form because the head argument is first.

StefanKarpinski · 2018-01-05T15:40:03Z

base/event.jl

@@ -341,7 +341,7 @@ end
 ## timer-based notifications

 """
-    Timer(delay, repeat=0)
+    Timer(delay; repeat = 0)


I think the repeat argument to Timer is kind of confusingly named – it seems like it gives a repeat count, but instead gives the interval of repetition. Perhaps it should be called interval instead with the same behavior? I.e. if given the timer repeats with the given interval, if not given the timer does not repeat. We might in the future want to add a repeat keyword argument which controls how many times a timer repeats. Of course, we might want to call such an argument count instead, but you see my point.

I was confused by repeat at first too. I'll change it to interval.

StefanKarpinski · 2018-01-05T15:43:29Z

base/pointer.jl

@@ -68,7 +68,7 @@ unsafe_convert(::Type{Ptr{T}}, a::AbstractArray{T}) where {T} = error("conversio

 # unsafe pointer to array conversions
 """
-    unsafe_wrap(Array, pointer::Ptr{T}, dims, own=false)
+    unsafe_wrap(Array, pointer::Ptr{T}, dims; own = false)


I think it's fine. It would have been preferable to do this without style changes and make style changes separately, but @bramtayl has been kind enough to do a large amount of tedious but important work here, so rather than asking him to do more, let's just focus on getting the meaningful content correct here and we can make our coding styles consistent to our hearts content after 1.0 is out.

StefanKarpinski · 2018-01-05T15:54:51Z

base/deprecated.jl

+@deprecate chop(s, head) chop(s, head = head)
+@deprecate chop(s, head, tail) chop(s, head = head, tail = tail)
+@deprecate tryparse(T, s, base) tryparse(T, s, base = base)
+@deprecate parse(T, s, base) parse(T, s, base = base)


Should these have T::Type{<:Integer}or something like that?

Sure, that sounds right

nalimilan · 2018-01-05T18:40:39Z

base/deprecated.jl

+@deprecate reverse!(v, start) reverse!(v, start = start)
+@deprecate reverse!(v, start, stop) reverse!(v, start = start, stop = stop)
+@deprecate reverse(v, start) reverse(v, start = start)
+@deprecate reverse(v, start, stop) reverse(v, start = start, stop = stop)


I wonder whether reverse should be kept as is. There's a pattern of passing start and stop indices as ~~keyword~~ positional arguments, e.g. with SubString and copy!. I prefer using keyword arguments for arguments which are of a "different nature" than other arguments (typically, options vs. data).

I don't understand...

What don't you understand exactly? :-)

@nalimilan did you typo "There's a pattern of passing start and stop indices as keyword arguments"? I think you meant "~~keyword~~ positional arguments".

Ah, thanks, that explains it!

I would be fine with leaving reverse and reverse! as they are but I don't hate this either.

I'm not super passionate either but its worth noting that if they are both optional arguments then you wouldn't be able to do something like reverse(v, stop = stop)

Yeah, you would have to pass start(v) before the stop index. But this remark also applies to functions with similar arguments, so I think we should keep them consistent (either way).

Do you have a list of such functions? It might be just worth changing all of them (if they are exported).

How about dropping that part for now so that we can get this merged now?

fredrikekre · 2018-01-16T20:33:02Z

base/event.jl


 Create a timer that wakes up tasks waiting for it (by calling [`wait`](@ref) on the timer object).

 Waiting tasks are woken after an intial delay of `delay` seconds, and then repeating with the given
-`repeat` interval in seconds. If `repeat` is equal to `0`, the timer is only triggered once. When
+`interval` in seconds. If `repeat` is equal to `0`, the timer is only triggered once. When


`repeat` -> `interval`

fredrikekre · 2018-01-16T20:33:44Z

base/file.jl

@@ -81,37 +81,44 @@ Temporarily changes the current working directory and applies function `f` befor
 """
 cd(f::Function) = cd(f, homedir())

+function checkmode(mode::Int) =
+    if !(0 <= mode <= 511)
+        ArgumentError("Mode must be between 0 and 511 = 0o777")


throw(...) ?

fredrikekre · 2018-01-16T20:35:06Z

base/loading.jl

@@ -88,7 +88,7 @@ macro return_if_file(path)
 end

 function find_package(name::String)
-    endswith(name, ".jl") && (name = chop(name, 0, 3))
+    endswith(name, ".jl") && (name = chop(name, head = 0, tail = 3))


fredrikekre · 2018-01-16T20:35:41Z

base/parse.jl

        return base
    end
    throw(ArgumentError("invalid base: base must be 2 ≤ base ≤ 62, got $base"))
 end

 """
-    tryparse(type, str, [base])
+    tryparse(type, str, base = base)


base = 0?

bramtayl · 2018-01-17T19:08:03Z

Thanks everyone for the detailed comments! I fixed some more things.

JeffBezanson · 2018-01-18T03:58:16Z

base/strings/util.jl

 ""
 ```
 """
 chop(s::AbstractString) = SubString(s, start(s), prevind(s, endof(s)))
-chop(s::AbstractString, head::Integer, tail::Integer) =
+chop(s::AbstractString; head::Integer, tail::Integer) =


Forgot the default values here.

JeffBezanson · 2018-01-18T03:58:48Z

Rebasing this now should give faster build times.

…into keywords_unlocked

fredrikekre · 2018-01-18T06:53:46Z

base/strings/util.jl

 ""
 ```
 """
 chop(s::AbstractString) = SubString(s, start(s), prevind(s, endof(s)))
-chop(s::AbstractString, head::Integer, tail::Integer) =
+chop(s::AbstractString; head::Integer = 0, tail::Integer = 1) =


This will overwrite the method on the line above, right?

Oh shoot I messed this up. If I understand correctly start(s) and prevind(s, endof(s)) are supposed to be optimizations for the default cause. If I'm understanding correctly constant propagation is not working right now for keyword arguments. So maybe I should comment out the first method and add back in the optimization if that ever happens?

JeffBezanson · 2018-01-18T15:56:55Z

base/deprecated.jl

+@deprecate parse(T::Type{<:Integer}, s, base) parse(T, s, base = base)
+@deprecate mkdir(path, mode) mkdir(path, mode = mode)
+import Filesystem.mkpath
+@deprecate mkpath(path, mode) mkpath(path, mode = mode)


Can wrap this line in @eval Filesystem begin ... end instead.

bramtayl · 2018-01-18T22:10:48Z

I'm having a heck of a time debugging this while rebasing it at the same time

JeffBezanson · 2018-01-20T02:42:58Z

Rebased and fixed some things: #25647

bramtayl added 23 commits January 4, 2018 01:31

keyword for world in method_exists

c035a41

repeat keyword for Timer

1cf51c3

keywords for all, imported in names

9b14556

syntax keyword for code_native

10b714f

expand_generated keyword for code_lowered

377fe70

Revert "expand_generated keyword for code_lowered"

6787c33

This reverts commit 377fe70.

base, pad keywords for digits

07479d8

base keyword for digits!

0c49105

overlap keyword for eachmatch

6aaa745

overlap keyword for matchall

d7984a3

head, tail keywords for chop

7eb8ee8

base keyword for tryparse

0677521

base keyword for parse

06fac95

start, stop keywords for reverse!

baba473

start, stop keywords for reverse

f9c3b30

trim keyword for dropzeros

ac418b8

trim keyword for dropzeros!

57c4ca0

mode keyword for mkdir

8f9a269

mode keyword for mkpath

eb880cb

mode keyword for chown

8903628

eol keyword for countlines

7b5041e

maxsize keyword for PipeBuffer

2b08a7a

own keyword for unsafe wrap

1eaf6bb

vtjnash reviewed Jan 4, 2018

View reviewed changes

ararslan reviewed Jan 4, 2018

View reviewed changes

StefanKarpinski reviewed Jan 5, 2018

View reviewed changes

nalimilan reviewed Jan 5, 2018

View reviewed changes

fredrikekre reviewed Jan 16, 2018

View reviewed changes

bramtayl added 6 commits January 17, 2018 13:58

backticks

9dfe16d

repeat -> interval

81485b4

throw

0e5f1e7

remove head = 0

7e148f8

base = 0

19520cb

move dropzeros to stdlib

d9d80c3

bramtayl added 2 commits January 17, 2018 14:10

Merge branch 'master' into keywords_unlocked

1d4cf3f

Merge branch 'master' into keywords_unlocked

8a31bc2

JeffBezanson reviewed Jan 18, 2018

View reviewed changes

bramtayl added 4 commits January 18, 2018 01:30

defaults for chop

44f28d6

Merge branch 'master' into keywords_unlocked

cc64366

Merge branch 'keywords_unlocked' of https://github.com/bramtayl/julia …

6e80f5d

…into keywords_unlocked

removed extraneous =

536a516

fredrikekre reviewed Jan 18, 2018

View reviewed changes

bramtayl added 4 commits January 18, 2018 02:39

remove overwritten chop

b819e65

sp

c8fe2f7

import mkpath

d541121

missing end

13f748a

JeffBezanson reviewed Jan 18, 2018

View reviewed changes

bramtayl added 3 commits January 18, 2018 11:30

eval Filesystem

39305e3

widen checkmode

d589948

eval filesystem for mkdir

59013c4

JeffBezanson mentioned this pull request Jan 20, 2018

keywords unlocked #25647

Merged

bramtayl closed this Jan 20, 2018

bramtayl mentioned this pull request Jan 22, 2018

chown keywords #25694

Closed

This pull request was closed.

Keywords unlocked part 1 #25395

Keywords unlocked part 1 #25395

Conversation

bramtayl commented Jan 4, 2018

bramtayl commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jan 5, 2018

StefanKarpinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbauman Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bramtayl Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bramtayl commented Jan 17, 2018

Choose a reason for hiding this comment

JeffBezanson commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bramtayl commented Jan 18, 2018

JeffBezanson commented Jan 20, 2018

nalimilan Jan 5, 2018 •

edited

Loading

mbauman Jan 6, 2018 •

edited

Loading

bramtayl Jan 6, 2018 •

edited

Loading