Skip to content

Commit

Permalink
Refactor some REPL issues (#552)
Browse files Browse the repository at this point in the history 
* Refactor: APIOptions should be a dictionary

* Fix `do_activate!` interface

* Update tests: APIOptions is a dictionary

* Allow more flexibility for REPL `do_<>` functions
  • Loading branch information
@00vareladavid @KristofferC
00vareladavid authored and KristofferC committed Aug 4, 2018
1 parent e9e320b commit 5b6fbb0
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 77 deletions.
117 changes: 53 additions & 64 deletions stdlib/Pkg/src/REPLMode.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ end
###########
# Options #
###########
#TODO should this opt be removed: ("name", :cmd, :temp => false)
struct OptionSpec
name::String
short_name::Union{Nothing,String}
Expand Down Expand Up @@ -101,15 +100,15 @@ struct ArgSpec
end
const CommandDeclaration = Tuple{CommandKind,
Vector{String}, # names
Function, # handler
Union{Nothing,Function}, # handler
Tuple{ArgClass, Vector{Int}}, # argument count
Vector{OptionDeclaration}, # options
Union{Nothing, Markdown.MD}, #help
}
struct CommandSpec
kind::CommandKind
names::Vector{String}
handler::Function
handler::Union{Nothing,Function}
argument_spec::ArgSpec # note: just use range operator for max/min
option_specs::Dict{String, OptionSpec}
help::Union{Nothing, Markdown.MD}
Expand Down Expand Up @@ -311,7 +310,6 @@ end
##############
const Token = Union{String, VersionRange, Rev}
const PkgArguments = Union{Vector{String}, Vector{PackageSpec}}
#TODO embed spec in PkgCommand?
struct PkgCommand
meta_options::Vector{Option}
spec::CommandSpec
Expand All @@ -321,14 +319,14 @@ struct PkgCommand
PkgCommand(meta_opts, cmd_name, opts, args) = new(meta_opts, cmd_name, opts, args)
end

const APIOption = Pair{Symbol, Any}
APIOptions(command::PkgCommand)::Vector{APIOption} =
const APIOptions = Dict{Symbol, Any}
APIOptions(command::PkgCommand)::Dict{Symbol, Any} =
APIOptions(command.options, command.spec.option_specs)

function APIOptions(options::Vector{Option},
specs::Dict{String, OptionSpec},
)::Vector{APIOption}
return map(options) do opt
)::Dict{Symbol, Any}
keyword_vec = map(options) do opt
spec = specs[opt.val]
# opt is switch
spec.is_switch && return spec.api
Expand All @@ -337,18 +335,9 @@ function APIOptions(options::Vector{Option},
# given opt wrapper
return spec.api.first => spec.api.second(opt.argument)
end
return Dict(keyword_vec)
end

function key_api(key::Symbol, api_opts::Vector{APIOption})
index = findfirst(x->x.first == key, api_opts)
if index !== nothing
return api_opts[index].second
end
end

set_default!(opt, api_opts::Vector{APIOption}) =
key_api(opt.first, api_opts) === nothing && push!(api_opts, opt)

function enforce_argument_order(args::Vector{Token})
prev_arg = nothing
function check_prev_arg(valid_type::DataType, error_message::AbstractString)
Expand Down Expand Up @@ -496,6 +485,8 @@ function PkgCommand(statement::Statement)::PkgCommand
return PkgCommand(meta_opts, statement.command, opts, args)
end

Context!(ctx::APIOptions)::Context = Types.Context!(collect(ctx))

#############
# Execution #
#############
Expand All @@ -519,15 +510,14 @@ function do_cmd(repl::REPL.AbstractREPL, input::String; do_rethrow=false)
end

function do_cmd!(command::PkgCommand, repl)
meta_opts = APIOptions(command.meta_options, meta_option_specs)
ctx = Context(meta_opts...)
context = APIOptions(command.meta_options, meta_option_specs)
spec = command.spec

# REPL specific commands
if spec.kind == CMD_HELP
return Base.invokelatest(do_help!, ctx, command, repl)
return Base.invokelatest(do_help!, command, repl)
elseif spec.kind == CMD_PREVIEW
ctx.preview = true
context[:preview] = true
cmd = command.arguments[1]
cmd_spec = get(command_specs, cmd, nothing)
cmd_spec === nothing &&
Expand All @@ -538,10 +528,15 @@ function do_cmd!(command::PkgCommand, repl)

# API commands
# TODO is invokelatest still needed?
Base.invokelatest(spec.handler, ctx, command.arguments, APIOptions(command))
api_opts = APIOptions(command)
if applicable(spec.handler, context, command.arguments, api_opts)
Base.invokelatest(spec.handler, context, command.arguments, api_opts)
else
Base.invokelatest(spec.handler, command.arguments, api_opts)
end
end

function do_help!(ctk::Context, command::PkgCommand, repl::REPL.AbstractREPL)
function do_help!(command::PkgCommand, repl::REPL.AbstractREPL)
disp = REPL.REPLDisplay(repl)
if isempty(command.arguments)
Base.display(disp, help)
Expand All @@ -562,82 +557,76 @@ function do_help!(ctk::Context, command::PkgCommand, repl::REPL.AbstractREPL)
end

# TODO set default Display.status keyword: mode = PKGMODE_COMBINED
function do_status!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption})
set_default!(:mode => PKGMODE_COMBINED, api_opts)
Display.status(ctx, key_api(:mode, api_opts))
end

# TODO remove the need to specify a handler function (not needed for REPL commands)
do_preview!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption}) = nothing
do_status!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions) =
Display.status(Context!(ctx), get(api_opts, :mode, PKGMODE_COMBINED))

# TODO , test recursive dependencies as on option.
function do_test!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption})
function do_test!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions)
foreach(arg -> arg.mode = PKGMODE_MANIFEST, args)
API.test(ctx, args; api_opts...)
API.test(Context!(ctx), args; collect(api_opts)...)
end

function do_registry_add!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption})
function do_registry_add!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions)
println("This is a dummy function for now")
println("My args are:")
for arg in args
println("- $arg")
end
end

do_precompile!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption}) =
API.precompile(ctx)
do_precompile!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions) =
API.precompile(Context!(ctx))

do_resolve!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption}) =
API.resolve(ctx)
do_resolve!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions) =
API.resolve(Context!(ctx))

do_gc!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption}) =
API.gc(ctx; api_opts...)
do_gc!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions) =
API.gc(Context!(ctx); collect(api_opts)...)

do_instantiate!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption}) =
API.instantiate(ctx; api_opts...)
do_instantiate!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions) =
API.instantiate(Context!(ctx); collect(api_opts)...)

do_generate!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption}) =
API.generate(ctx, args[1])
do_generate!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions) =
API.generate(Context!(ctx), args[1])

do_build!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption}) =
API.build(ctx, args; api_opts...)
do_build!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions) =
API.build(Context!(ctx), args; collect(api_opts)...)

do_rm!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption}) =
API.rm(ctx, args; api_opts...)
do_rm!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions) =
API.rm(Context!(ctx), args; collect(api_opts)...)

do_free!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption}) =
API.free(ctx, args; api_opts...)
do_free!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions) =
API.free(Context!(ctx), args; collect(api_opts)...)

do_up!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption}) =
API.up(ctx, args; api_opts...)
do_up!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions) =
API.up(Context!(ctx), args; collect(api_opts)...)

function do_activate!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption})
# TODO: Remove the ctx argument to this function.
function do_activate!(args::PkgArguments, api_opts::APIOptions)
if isempty(args)
return API.activate(nothing)
else
return API.activate(args[1])
end
end

function do_pin!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption})
function do_pin!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions)
for arg in args
# TODO not sure this is correct
if arg.version.ranges[1].lower != arg.version.ranges[1].upper
cmderror("pinning a package requires a single version, not a versionrange")
end
end
API.pin(ctx, args; api_opts...)
API.pin(Context!(ctx), args; collect(api_opts)...)
end

function do_add!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption})
push!(api_opts, :mode => :add)
API.add_or_develop(ctx, args; api_opts...)
function do_add!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions)
api_opts[:mode] = :add
API.add_or_develop(Context!(ctx), args; collect(api_opts)...)
end

function do_develop!(ctx::Context, args::PkgArguments, api_opts::Vector{APIOption})
push!(api_opts, :mode => :develop)
API.add_or_develop(ctx, args; api_opts...)
function do_develop!(ctx::APIOptions, args::PkgArguments, api_opts::APIOptions)
api_opts[:mode] = :develop
API.add_or_develop(Context!(ctx), args; collect(api_opts)...)
end

######################
Expand Down Expand Up @@ -950,7 +939,7 @@ julia is started with `--startup-file=yes`.
""",
),( CMD_HELP,
["help", "?"],
do_help!,
nothing,
(ARG_RAW, []),
[],
md"""
Expand Down Expand Up @@ -1201,7 +1190,7 @@ Deletes packages that cannot be reached from any existing environment.
""",
),( CMD_PREVIEW,
["preview"],
do_preview!,
nothing,
(ARG_RAW, [1]),
[],
md"""
Expand Down
8 changes: 8 additions & 0 deletions stdlib/Pkg/src/Types.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,14 @@ Base.@kwdef mutable struct Context
old_pkg2_clone_name::String = ""
end

function Context!(kw_context::Vector{Pair{Symbol,Any}})::Context
ctx = Context()
for (k, v) in kw_context
setfield!(ctx, k, v)
end
return ctx
end

function Context!(ctx::Context; kwargs...)
for (k, v) in kwargs
setfield!(ctx, k, v)
Expand Down
20 changes: 7 additions & 13 deletions stdlib/Pkg/test/repl.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -785,26 +785,20 @@ end
Pkg.REPLMode.Option("rawnum", "5"),
], specs)

@test Pkg.REPLMode.key_api(:foo, api_opts) === nothing
@test Pkg.REPLMode.key_api(:mode, api_opts) == Pkg.Types.PKGMODE_MANIFEST
@test Pkg.REPLMode.key_api(:level, api_opts) == Pkg.Types.UPLEVEL_PATCH
@test Pkg.REPLMode.key_api(:num, api_opts) == "5"
@test get(api_opts,:foo,nothing) === nothing
@test get(api_opts,:mode,nothing) == Pkg.Types.PKGMODE_MANIFEST
@test get(api_opts,:level,nothing) == Pkg.Types.UPLEVEL_PATCH
@test get(api_opts,:num,nothing) == "5"

api_opts = Pkg.REPLMode.APIOptions([
Pkg.REPLMode.Option("project"),
Pkg.REPLMode.Option("patch"),
Pkg.REPLMode.Option("plus", "5"),
], specs)

@test Pkg.REPLMode.key_api(:mode, api_opts) == Pkg.Types.PKGMODE_PROJECT
@test Pkg.REPLMode.key_api(:level, api_opts) == Pkg.Types.UPLEVEL_PATCH
@test Pkg.REPLMode.key_api(:num, api_opts) == 6

@test Pkg.REPLMode.key_api(:foo, api_opts) === nothing
Pkg.REPLMode.set_default!(:foo => "bar", api_opts)
@test Pkg.REPLMode.key_api(:foo, api_opts) == "bar"
Pkg.REPLMode.set_default!(:level => "bar", api_opts)
@test Pkg.REPLMode.key_api(:level, api_opts) == Pkg.Types.UPLEVEL_PATCH
@test get(api_opts,:mode,nothing) == Pkg.Types.PKGMODE_PROJECT
@test get(api_opts,:level,nothing) == Pkg.Types.UPLEVEL_PATCH
@test get(api_opts,:num,nothing) == 6
end

@testset "meta option errors" begin
Expand Down

0 comments on commit 5b6fbb0

Please sign in to comment.