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

required keyword arguments #586

Merged
merged 8 commits into from
Jul 18, 2018
Merged

required keyword arguments #586

merged 8 commits into from
Jul 18, 2018

Conversation

timziebart
Copy link
Contributor

@timziebart timziebart commented Jul 4, 2018

Following up on the comment by @Keno in this PR JuliaLang/METADATA.jl#15489 I added the functionality to understand required keyword arguments to @compat. Basically that means

@compat f(;x) = [...]

is equivalent to

f(;x=error("RequiredKeywordArgumentError: `x` is a required keyword argument, please provide `x = ...`.")) = [...]

As this is my first submission to a JuliaLang rep, could you check that the style etc. matches what you would expect?

@KristofferC
Copy link
Member

Restarting CI.

@KristofferC KristofferC closed this Jul 4, 2018
@KristofferC KristofferC reopened this Jul 4, 2018
@fredrikekre
Copy link
Member

I suggest that you add a

struct UndefKeywordError <: Exception
    kw
end
Base.showerror(io::IO, e::UndefKeywordError) = print(io, "keyword argument $(e.kw) not assigned")

to make it look the same as on Julia 0.7.

test/runtests.jl Outdated
@test func2(3) == 3 # using the function works
@compat func3(;y) = y
@test func3(y=2) == 2 # using the function works
try
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of try-catch you can use @test_throws, for example

julia> f(;y) = y
f (generic function with 1 method)

julia> @test_throws UndefKeywordError f()
Test Passed
      Thrown: UndefKeywordError

@timziebart
Copy link
Contributor Author

thanks @fredrikekre , I will add that

@timziebart
Copy link
Contributor Author

The errors seem to be due to the export UndefKeywordError line. Should I simply not export it (which would make it a different usage than in 0.7) or should I add a condition (see below)?

if !isdefined(:UndefKeywordError)
    struct UndefKeywordError <: Exception
        kw
    end
    Base.showerror(io::IO, e::UndefKeywordError) = print(io, "keyword argument $(e.kw) not assigned")
    export UndefKeywordError
end

?

@fredrikekre
Copy link
Member

Yes, you can use !isdefined(Base, :UndefKeywordError), and also in the macro implementation, since this should just pass through @compat unchanged on julia 0.7

@timziebart timziebart changed the title required keyword arguments working required keyword arguments Jul 4, 2018
@timziebart
Copy link
Contributor Author

Thanks, seems to work now!

struct UndefKeywordError <: Exception
kw
end
Base.showerror(io::IO, e::UndefKeywordError) = print(io, "keyword argument $(e.kw) not assigned")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this should also print the exception type, so

print(io, "UndefKeywordError: keyword argument $(e.kw) not assigned")

as it does on Julia v0.7:

julia> f(;y) = y
f (generic function with 1 method)

julia> f()
ERROR: UndefKeywordError: keyword argument y not assigned

otherwise LGTM

@timziebart
Copy link
Contributor Author

True, corrected that.

@stevengj
Copy link
Member

stevengj commented Jul 5, 2018

Needs a README item.

@timziebart
Copy link
Contributor Author

@stevengj: absolutely. I added a description in the README, is it okai like this?

README.md Outdated
@@ -79,6 +79,8 @@ Currently, the `@compat` macro supports the following syntaxes:
`foo(::CartesianRange{CartesianIndex{N}})` ([#20974]). Note that
`CartesianRange` now has two type parameters, so using them as
fields in other `struct`s requires manual intervention.

* required keyword arguments: e.g. `@compat foo(;x, y)` makes `x` and `y` required keyword arguments. When calling `foo` an error is thrown if `x` and `y` are not explictly provided
Copy link
Member

@stevengj stevengj Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some fixes:

* Required keyword arguments ([#25830]). For example, `@compat foo(; x, y)` makes `x` and `y` required keyword arguments: when calling `foo`, an error is thrown if `x` or `y` is not explicitly provided.

Copy link
Member

@stevengj stevengj Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The link to JuliaLang/julia#25830 is added if you run julia /path/to/julia/doc/NEWS-update.jl README.jl to run https://github.com/JuliaLang/julia/blob/master/doc/NEWS-update.jl)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed the update README.md.

@timziebart
Copy link
Contributor Author

@stevengj I responded to your comment and updated the README.md a few days ago. Do you agree with the result?

@stevengj stevengj merged commit a9b0a3d into JuliaLang:master Jul 18, 2018
@stevengj
Copy link
Member

Thanks!

function symbol2kw(sym::Symbol)
Expr(:kw, sym, Expr(:call, throw, UndefKeywordError(sym)))
end
symbol2kw(arg) = arg
Copy link
Member

@stevengj stevengj Oct 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't handle the case where the argument has a type. It should probably be:

arg2kw(arg, sym) = Expr(:kw, arg, Expr(:call, throw, UndefKeywordError(sym)))
symbol2kw(arg::Symbol) = arg2kw(arg, arg)
symbol2kw(arg::Expr) = isexpr(arg, :(::)) ? arg2kw(arg, arg.args[1]) : arg
symbol2kw(arg) = arg

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timziebart
Copy link
Contributor Author

timziebart commented Oct 21, 2018

Yes, that seems reasonable, should I update the PR? Can I even or should this be a new PR?

martinholters added a commit that referenced this pull request Oct 13, 2019
* Bump required Julia version to 1.0

* Remove compatibility support code for:
  * `at-__MODULE__` (from #363)
  * `devnull`, `stdin`, `stdout`, and `stderr` from #499
  * `at-nospecialize` (from #385 and #409)
  * `isabstracttype` and `isconcretetype` (from #477)
  * `invokelatest` from #424
  * array-like access to `Cmd` from #379
  * `Val(n)` and `ntuple`/`reshape` with `Val` from #381 and #399
  * `logdet(::Any)` fallback from #382
  * `chol(::UniformScaling)` from #382
  * `pushfirst!`, `popfirst!` from #444
  * `fieldcount` from #386
  * `read(obj, ::Type{String})` from #385 and #580
  * `InexactError`, `DomainError`, and `OverflowError` constructors from #393
  * `corrected` kw arg to `cov` from #401
  * `adjoint` from #401
  * `partialsort` from #401
  * `pairs` from #428
  * `AbstractRange` from #400
  * `rtoldefault` from #401
  * `Dates.Period` rounding from #462
  * `IterativeEigensolvers` from #435
  * `occursin` from #520
  * `Char` concatenation from #406
  * `BitSet` from #407
  * `diagm` and `spdiagm` with pairs from #408
  * `Array` c'tors from `UniformScaling` from #412 and #438
  * `IOContext` ctor taking pairs from #427
  * `undef` from #417 and #514
  * `get` on `ENV` from #430
  * `ComplexF...` from #431
  * `tr` from #614
  * `textwidth` from #644
  * `isnumeric` from #543
  * `AbstractDict` from #435
  * `axes` #435 and #442
  * `Nothing` and `Cvoid` from #435
  * `Compat.SuiteSparse` from #435
  * `invpermute!` from #445
  * `replace` with a pair from #445
  * `copyto!` from #448
  * `contains` from #452
  * `CartesianIndices` and `LinearIndices` from #446, #455, and #524
  * `findall` from #466 (and #467).
  * `argmin` and `argmax` from #470, #472, and #622
  * `parentmodule` from #461
  * `codeunits` from #474
  * `nameof` from #471
  * `GC` from #477
  * `AbstractDisplay` from #482
  * `bytesavailable` from #483
  * `firstindex` and `lastindex` from #480 and #494
  * `printstyled` from #481
  * `hasmethod` from #486
  * `objectid` from #486
  * `Compat.find*` from #484 and #513
  * `repr` and `showable` from #497
  * `Compat.names` from #493 and #505
  * `Compat.round` and friends #500, #530, and #537
  * `IOBuffer` from #501 and #504
  * `range` with kw args and `LinRange` from #511
  * `cp` and `mv` from #512
  * `indexin` from #515
  * `isuppercase` and friends from #516
  * `dims` and `init` kwargs from #518, #528, #590, #592, and #613
  * `selectdim` from #522 and #531
  * `repeat` from #625
  * `fetch(::Task)` from #549
  * `isletter` from #542
  * `isbitstype` from #560
  * `at-cfunction` from #553 and #566
  * `codeunit` and `thisind` and friends from #573
  * `something` from #562
  * `permutedims` from #582
  * `atan` from #574
  * `split` and `rsplit` from #572
  * `mapslices` from #588
  * `floatmin` and `floatmax` from #607
  * `dropdims` from #618
  * required keyword arguments from #586
  * `CartesianRange` in `at-compat` from #377
  * `finalizer` from #416
  * `readline`, `eachline`, and `readuntil` from #477, #541, and #575
  * curried `isequal`, `==`, and `in` from #517
  * `Some` from #435 and #563
  * `at-warn` and friends from #458

* Remove old deprecations

* Deprecate:
  * `Compat.Sockets` from #545 and #594
  * `TypeUtils` from #304
  * `macros_have_sourceloc` from #355
  * `Compat.Sys` from #380, #433, and #552
  * `Compat.MathConstants` from #401
  * `Compat.Test`, `Compat.SharedArrays`, `Compat.Mmap`, and `Compat.DelimitedFiles` from #404
  * `Compat.Dates` from #413
  * `Compat.Libdl` from #465 (and #467)
  * `AbstractDateTime` from #443
  * `Compat.Printf` from #435
  * `Compat.LinearAlgebra` from #463
  * `Compat.SparseArrays` from #459
  * `Compat.Random` from #460, #601, and #647
  * `Compat.Markdown` from #492
  * `Compat.REPL` from #469
  * `Compat.Serialization` from #473
  * `Compat.Statistics` from #583
  * `Fix2` from #517
  * `Compat.Base64` from #418
  * `Compat.Unicode` from #432 and #507
  * `notnothing` from #435 and #563
  * `Compat.IteratorSize` and `Compat.IteratorEltype` from #451
  * `enable_debug(::Bool)` from #458
  * `Compat.Distributed` from #477
  * `Compat.Pkg` from #485
  * `Compat.InteractiveUtils` from #485
  * `Compat.LibGit2` from #487
  * `Compat.UUIDs` from #490
  * `Compat.qr` from #534
  * `Compat.rmul!` from #546
  * `Compat.norm` abd friends from #577

* Remove obsolete README entry, missed in #385

* Remove obsolete tests (e.g. missed in #372)

* Remove obsolete `VERSION` conditionals and some minor clean-up
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants