[RFC] Make command spec more declarative #509

00vareladavid · 2018-07-17T22:21:27Z

Still a WIP, but I'd be interested if anyone thinks this is a worthwhile change?

My end goal would be a more declarative specification of commands (e.g. specify a list of valid flags for a command , specify the number of expected arguments, ... and have the parser check those declarative constraints).

It will certainly be 'heavier' than the current code but it should be insignificant for interactive input sizes.

codecov-io · 2018-07-17T22:38:10Z

Codecov Report

Merging #509 into master will increase coverage by 0.63%.
The diff coverage is 92.7%.

@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
+ Coverage   90.67%   91.31%   +0.63%     
==========================================
  Files          18       18              
  Lines        2896     2948      +52     
==========================================
+ Hits         2626     2692      +66     
+ Misses        270      256      -14

Impacted Files	Coverage Δ
src/API.jl	`80.81% <100%> (-6.19%)`	⬇️
src/REPLMode.jl	`92.7% <92.54%> (+11.53%)`	⬆️
src/versions.jl	`98.06% <0%> (-0.02%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0621d86...7c67885. Read the comment docs.

KristofferC · 2018-07-18T08:29:52Z

Yeah, I think this is a good idea and something I have thought about every time I add a new REPL command :).

00vareladavid · 2018-07-18T19:27:05Z

Ok, great!

StefanKarpinski · 2018-07-19T16:58:17Z

src/REPLMode.jl

+    ,("patch", :cmd, :switch)
+    ,("fixed", :cmd, :switch)
+    ,("coverage", :cmd, :switch)
+    ,("name", :cmd, :switch)


Leading commas like this are anti-idiomatic in Julia; Julia allows a comma after the last element, so just end every line with a comma. Note that vectors can also have no commas at all and just use newlines, but you have to be consistent.

Thanks. I'll go with the trailing commas. (Also, I didn't know about the no commas for vectors. Pretty cool!)

It's technically a different syntax: with commas it's a vector construction syntax, without commas it's a vertical concatenation syntax, but in this case the result is the same. If the elements are themselves vectors or matrices, the result can be different, e.g.:

julia> [[1, 2], [3, 4, 5], [6, 7]] 3-element Array{Array{Int64,1},1}: [1, 2] [3, 4, 5] [6, 7] julia> [[1, 2] [3, 4, 5] [6, 7]] 7-element Array{Int64,1}: 1 2 3 4 5 6 7

Ah, I see the difference now: no commas will end up calling vcat. Thanks for explaining

00vareladavid · 2018-07-28T22:30:33Z

Hey guys, I would appreciate any feedback on the status of this PR. The reason I'm asking is that it is becoming painful to keep up to date with the master since REPLMode.jl diverges significantly.

As a convincing example in workflow difference you can compare: my merge
with the original.

KristofferC · 2018-07-30T11:13:41Z

Sorry, for lack of comments. This looks good but we need to test if it introduces any significant changes to time for first call (e.g. pkg> st.

Likely need to regenerate some of the precompile statements as well but that's on me.

00vareladavid · 2018-07-30T19:42:56Z

Ok, I think I can test that

00vareladavid · 2018-07-30T21:03:42Z

I ran some basic tests like this:

import Pkg
Pkg.activate("/home/nur0n/pkg/Pkg.jl") # use the same project file
@time Pkg.REPLMode.pkg"st"

I think this should test the right thing as the changes for this PR don't take effect until you hit REPLMode.do_cmd.

I profiled manually 5 times, then took the average:

master took about ~5.1 seconds on the first run.
refactor/parse took about ~4.7 seconds on the first run.
both take ~0.03 seconds for subsequent runs.

Not the most thorough tests, but I find them reassuring (given that they are testing the right thing...).

fredrikekre · 2018-07-30T21:11:26Z

I haven't looked at the implementation here, but how would "double" commands like registry add (#453 (comment)) fit into this design?

StefanKarpinski · 2018-07-30T22:07:46Z

I wonder if add shouldn't just be an implicit shorthand for package add...

00vareladavid · 2018-07-30T22:11:45Z

Excellent point!

The closest thing right now would be to use options:
registry rm Registry=ae0cb698-197b-42ec-a0a0-4f871aea6013 -> registry --rm Registry=ae0cb698-197b-42ec-a0a0-4f871aea6013.
The declaration would be:

(       CMD_REGISTRY,
        ["registry"],
        do_registry!,
        (ARG_PKG, []),
        [
            ("add", OPT_SWITCH, :cmd => :add),
            ("rm", OPT_SWITCH, :cmd => :rm),
        ],
        md"",
        )

That's the current design though, I hadn't considered explicit support for subcommands. Extending support for them should be easy; it will be my next TODO. What I have in mind is declaring each subcommand:

(       CMD_REGISTRY_ADD,
        ["add"],
        do_registry_add!,
        (ARG_PKG, []),
        [],
        md"",
        ),(
        CMD_REGISTRY_RM,
       ["rm"],
       ...

The parser will check for a super command and a sub command, with "package" being the implicit super command.

KristofferC · 2018-07-31T09:01:58Z

Let's merge this to prevent more merge conflicts and then I can work on fixing up the precompilation if needed.

fredrikekre · 2018-07-31T10:39:29Z

Thanks for adding the registry placeholders!

00vareladavid · 2018-07-31T13:39:47Z

@fredrikekre welcome!

fredrikekre · 2018-07-31T14:19:53Z

src/REPLMode.jl

-    API.gc(ctx)
+function do_cmd!(command::PkgCommand, repl)
+    meta_opts = APIOptions(command.meta_options, meta_option_specs)
+    ctx = Context(meta_opts...)


I think this will fail sometimes if there is no active project? Thats why the call to do_activate! was called before creating the context before this PR. See #517 (comment)

Is this still true @KristofferC ? If so, does the help command not work either? It is called after Context() both before and after this PR.

Do you mean when Base.ACTIVE_PROJECT[] == nothing? How would I test that failure case? Maybe:

Base.ACTIVE_PROJECT[] = nothing Pkg.REPLMode.pkg"activate"

It is the call to EnvCache that hits

Pkg.jl/src/Types.jl

Line 237 in e1627a6

project_file == nothing && error("no active project")

Can be reproduced with:

julia> using Pkg julia> empty!(LOAD_PATH) 0-element Array{String,1} julia> Base.ACTIVE_PROJECT[] = nothing julia> Base.HOME_PROJECT[] = nothing julia> Pkg.Types.EnvCache() ERROR: no active project Stacktrace: [1] error(::String) at ./error.jl:33 [2] find_project_file(::Nothing) at /home/fredrik/.julia/dev/Pkg/src/Types.jl:237 [3] Pkg.Types.EnvCache(::Nothing) at /home/fredrik/.julia/dev/Pkg/src/Types.jl:279 (repeats 2 times) [4] top-level scope at none:0 julia> pkg"activate" ERROR: no active project Stacktrace: [1] error(::String) at ./error.jl:33 [2] find_project_file(::Nothing) at /home/fredrik/.julia/dev/Pkg/src/Types.jl:237 [3] Pkg.Types.EnvCache(::Nothing) at /home/fredrik/.julia/dev/Pkg/src/Types.jl:279 (repeats 2 times) [4] Pkg.Types.Context() at ./none:0 [5] do_cmd!(::Pkg.REPLMode.PkgCommand, ::Pkg.REPLMode.MiniREPL) at /home/fredrik/.julia/dev/Pkg/src/REPLMode.jl:523 [6] #do_cmd#32(::Bool, ::Function, ::Pkg.REPLMode.MiniREPL, ::String) at /home/fredrik/.julia/dev/Pkg/src/REPLMode.jl:507 [7] (::getfield(Pkg.REPLMode, Symbol("#kw##do_cmd")))(::NamedTuple{(:do_rethrow,),Tuple{Bool}}, ::typeof(Pkg.REPLMode.do_cmd), ::Pkg.REPLMode.MiniREPL, ::String) at ./none:0 [8] top-level scope at none:0

And the help command fails too (which it also did before this PR).

julia> pkg"help" ERROR: no active project Stacktrace: [1] error(::String) at ./error.jl:33 [2] find_project_file(::Nothing) at /home/fredrik/.julia/dev/Pkg/src/Types.jl:237 [3] Pkg.Types.EnvCache(::Nothing) at /home/fredrik/.julia/dev/Pkg/src/Types.jl:279 (repeats 2 times) [4] Pkg.Types.Context() at ./none:0 [5] do_cmd!(::Pkg.REPLMode.PkgCommand, ::Pkg.REPLMode.MiniREPL) at /home/fredrik/.julia/dev/Pkg/src/REPLMode.jl:523 [6] #do_cmd#32(::Bool, ::Function, ::Pkg.REPLMode.MiniREPL, ::String) at /home/fredrik/.julia/dev/Pkg/src/REPLMode.jl:507 [7] (::getfield(Pkg.REPLMode, Symbol("#kw##do_cmd")))(::NamedTuple{(:do_rethrow,),Tuple{Bool}}, ::typeof(Pkg.REPLMode.do_cmd), ::Pkg.REPLMode.MiniREPL, ::String) at ./none:0 [8] top-level scope at none:0

Yea that is probably better. Things like registry add won't need the Context either.

Ok, nice catch btw!

Just to clarify; you mean that the do_[]! functions create the Context then?

I think it would be best if devs deal with the context (and APIOptions) in the REPL layer as a Dict{Symbol,Any} (like here or here). With Context! here. This should allow us to deal with an incomplete context before we hit the API.

When we finally hit the API, I think we will end up passing collect(ctx) or Context!(collect(ctx)) depending on the primary way we want users to use the API (I'm not sure which would be better yet).

* Refactor `REPLMode.parse_quotes`

00vareladavid added 4 commits July 11, 2018 17:18

Refactor REPLMode.parse_quotes

fb2e71b

Merge remote-tracking branch 'upstream/master' into 00/refactor/repl

65ebcf7

Change parsing to a "multi-pass" design

a4d211c

Merge remote-tracking branch 'upstream/master' into 00/refactor/parse

6434f51

00vareladavid added 9 commits July 17, 2018 15:53

Break up statements early

447a170

parse will return structured Statements

3492f63

Remove unused code

e5dfdeb

Fix: pop should be popfirst

ba247b1

Tweak parse structs

75810e9

parse returns a vector of statements

f0f2ea1

do_cmd executes a series of statements

dbe549f

do_statement! should not parse

4e924fd

Fix: arguments are Tokens

f42ad52

00vareladavid added 8 commits July 18, 2018 13:57

Introduce rough draft for CommandSpec

4b3aee5

Move some functions around

2510ecb

Add command_spec initialization. Dispatch to handler, not api

85d3049

No more need for Command struct

1208b47

Rename arg_count to arg_spec

40f03d9

Cache data while initializing command spec

f9504fd

BreakToken no longer needed (breaking happens before tokenization)

67ce303

Introduce rough draft for option spec

0e91ae1

StefanKarpinski reviewed Jul 19, 2018

View reviewed changes

00vareladavid added 5 commits July 20, 2018 12:54

Make spec declarations more idiomatic

b34fde5

refactor init_option_spec

eba74ff

Tweak OptionDeclaration types

64fa69b

Refactor Option input checks

5fb7ef5

Add some tests for the parser

78853fd

00vareladavid added 5 commits July 28, 2018 14:36

Merge commit '0e83825' into 00/refactor/parse

0f04545

Update REPLMode.do_develop! based on last merge

6e989f7

Merge commit '66aaa3c' into 00/refactor/parse

93d2629

Merge commit '0621d86' into 00/refactor/parse

caa573c

Missed one

1451d56

00vareladavid added 6 commits July 29, 2018 13:57

Add more tests

f48e26d

Rename init_command_spec -> CommandSpecs

5ec4cff

Refactor APIOption handler functions

0cdcb6d

Refactor REPL command handler interface

90ba20d

Fix: update tests

d64119f

More tests for api options

34ab275

Add support for subcommands

7c67885

KristofferC merged commit 8c8cd83 into JuliaLang:master Jul 31, 2018

00vareladavid mentioned this pull request Jul 31, 2018

Refactor REPLMode.parse_quotes #498

Closed

fredrikekre reviewed Jul 31, 2018

View reviewed changes

This was referenced Jul 31, 2018

parse_env should be something else? #549

Closed

Make APIOptions a dictionary (instead of a keyword vector) #550

Closed

fredrikekre mentioned this pull request Jul 31, 2018

pkg> activate and pkg> help should work even without an active project #551

Closed

00vareladavid deleted the 00/refactor/parse branch August 15, 2018 21:46

fredrikekre pushed a commit that referenced this pull request Oct 15, 2018

[RFC] Make command spec more declarative (#509)

fc57b42

* Refactor `REPLMode.parse_quotes`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Make command spec more declarative #509

[RFC] Make command spec more declarative #509

00vareladavid commented Jul 17, 2018

codecov-io commented Jul 17, 2018 •

edited

Loading

KristofferC commented Jul 18, 2018

00vareladavid commented Jul 18, 2018

StefanKarpinski Jul 19, 2018

00vareladavid Jul 20, 2018

StefanKarpinski Jul 20, 2018 •

edited

Loading

00vareladavid Jul 20, 2018

00vareladavid commented Jul 28, 2018

KristofferC commented Jul 30, 2018

00vareladavid commented Jul 30, 2018

00vareladavid commented Jul 30, 2018 •

edited

Loading

fredrikekre commented Jul 30, 2018

StefanKarpinski commented Jul 30, 2018

00vareladavid commented Jul 30, 2018 •

edited

Loading

KristofferC commented Jul 31, 2018

fredrikekre commented Jul 31, 2018

00vareladavid commented Jul 31, 2018

fredrikekre Jul 31, 2018

fredrikekre Jul 31, 2018

00vareladavid Jul 31, 2018

fredrikekre Jul 31, 2018

fredrikekre Jul 31, 2018

fredrikekre Jul 31, 2018

00vareladavid Jul 31, 2018

fredrikekre Jul 31, 2018

00vareladavid Jul 31, 2018 •

edited

Loading

00vareladavid Jul 31, 2018

[RFC] Make command spec more declarative #509

[RFC] Make command spec more declarative #509

Conversation

00vareladavid commented Jul 17, 2018

codecov-io commented Jul 17, 2018 • edited Loading

Codecov Report

KristofferC commented Jul 18, 2018

00vareladavid commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

00vareladavid commented Jul 28, 2018

KristofferC commented Jul 30, 2018

00vareladavid commented Jul 30, 2018

00vareladavid commented Jul 30, 2018 • edited Loading

fredrikekre commented Jul 30, 2018

StefanKarpinski commented Jul 30, 2018

00vareladavid commented Jul 30, 2018 • edited Loading

KristofferC commented Jul 31, 2018

fredrikekre commented Jul 31, 2018

00vareladavid commented Jul 31, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

00vareladavid Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 17, 2018 •

edited

Loading

StefanKarpinski Jul 20, 2018 •

edited

Loading

00vareladavid commented Jul 30, 2018 •

edited

Loading

00vareladavid commented Jul 30, 2018 •

edited

Loading

00vareladavid Jul 31, 2018 •

edited

Loading