Accept and parse EDN Query and Transact commands (#453) by fluffyemily · Pull Request #465 · mozilla/mentat

fluffyemily · 2017-05-16T15:58:26Z

depends on #463

ncalexan

Overall this is looking good.

However, I found the expression of continuation with more: Option<Command> difficult to process. Can you fold these patches down a little, so that I can see the final form?

ncalexan · 2017-05-16T23:55:27Z

+    }
+
+    #[test]
+    fn test_transact_parser_complete_edn() {


A transaction is a vector-of-vectors or a vector-of-maps, in order to handle multiple assertions. So

.t [[:db/add "t" ...] [:db/add "r" ...]]

ncalexan · 2017-05-16T23:56:43Z

 }

 pub fn command(s: &str) -> Command {
+    let skip_spaces = skip_many(space());


This is the spaces() parser from combine::char. I've found that let p = || ... and then p() is more natural than cloning the parser.

ncalexan · 2017-05-16T23:58:51Z

+        ]))
    .skip(eof())
    .parse(s)
-    .map(|x| x.0)


Eh? This seems like unnecessary whitespace.

ncalexan · 2017-05-16T23:59:30Z

+                    .and(many::<Vec<_>, _>(try(any()))));
+
+    let query_parser = string(QUERY_COMMAND)
+                    .and(edn_arg_parser.clone())


See my other commit with .skip and .then for avoiding these indexing operations.

ncalexan · 2017-05-17T00:00:14Z

+
+    let edn_arg_parser = skip_spaces.clone()
+                    .and(opening_brace_parser.clone()
+                    .and(many::<Vec<_>, _>(try(any()))));


Can you explain why you need to try here? any will either consume and succeed or not consume and fail; there shouldn't be consume and fail.

I am not sure why, but if I leave the try out, I get no output at all from the many.

ncalexan · 2017-05-17T00:00:55Z

 pub static HELP_COMMAND: &'static str = &"help";
 pub static OPEN_COMMAND: &'static str = &"open";
 pub static CLOSE_COMMAND: &'static str = &"close";
+pub static QUERY_COMMAND: &'static str = &"q";


Can we have synonyms, ".query" for ".q", ".transact" for ".t"?

There must be some way that I can retain the eof() after the `then` that means I don't have to move the skip on spaces and eof Make in process command storing clearer. Add comments around in process commands. Add alternative commands for transact/t and query/q

ncalexan

OK, this is getting cleaner. I'd like to understand what the advantage of checking for argument counts, etc, during parsing is. Is your goal to maintain user input that parses but isn't semantically correct across invocations, allowing the user to edit their work in place?

I think that parsing and interpretation should be separate stages; and that user's should use up/down (maybe via rlwrap) to get their previous, incorrect input. That might simplify a lot of the error handling you're baking into the parser.

ncalexan · 2017-05-23T19:19:46Z


        loop {
-            let res = input.read_input(if more.is_some() { MORE_PROMPT } else { DEFAULT_PROMPT });
+            let res = input.read_input(more.clone());


I'm surprised you need this clone, since you're joining strings in read_input, not re-using them.

ncalexan · 2017-05-23T19:22:04Z

+            Some(Command::Transact(args)) => {
+                Command::Transact(args + " " + &line)
+            },
+            _ => {


Prefer to explicitly match Some(...) and None here, so that the compiler will tell you when you need to handle cases. I feel like there's an abstraction around continuable commands vs. non-continuable commands missing here, but it's your choice how to express this.

ncalexan · 2017-05-23T19:24:02Z

+                Command::Transact(args + " " + &line)
+            },
+            _ => {
+                try!(command(&self.buffer))


Prefer ...? to try!(...); it's the modern expression in Rust. Are you really only doing this evaluation for the error result? You should consume the result of any Result, using let _ = ... if needed; the compiler should be warning you that you're dropping a result on the floor here.

ncalexan · 2017-05-23T19:30:37Z

+    let edn_arg_parser = || spaces()
+                    .with(try(string("["))
+                        .or(try(string("{")))
+                        .then(|d| parser(move |input| {


If I understand correctly, you're trying to parse any line prefixed by optional space and either [ or {. I think you can do that with:

spaces() .with(look_ahead(string("[").or(string("{"))) .with(many(any()))

Using then and a special parser like you do is ingenious but definitely not a natural expression using combine :) You might also do this as

spaces().then(many(any())).and_then(|line| { if line.starts_with("[") || line.starts_with("{") { Ok(...) } else { Err(...) } })

but then you need to know a lot more about error handling in combine. Stick to the defined combinators for easiest use.

ncalexan · 2017-05-23T19:34:02Z

-pub static QUERY_COMMAND: &'static str = &"q";
-pub static TRANSACT_COMMAND: &'static str = &"t";
+pub static QUERY_COMMAND: &'static str = &"query";
+pub static ALT_QUERY_COMMAND: &'static str = &"q";


If you label these SHORT_ and LONG_, your try clauses will read more clearly.

ncalexan · 2017-05-23T19:37:34Z


+    #[test]
+    fn test_open_parser_no_args_trailing_whitespace() {
+        let input = ".open ";


I see that expect_err is in 1.17.0 stable, so we can use it -- but we need to bump

mentat/build/version.rs

Line 19 in 8a8fced

static MIN_VERSION: &'static str = ">= 1.15.1";

as a Pre: part to stop folks failing here (as I just did locally!).

ncalexan · 2017-05-23T19:43:39Z

    /// Runs the REPL interactively.
    pub fn run(&mut self) {
-        let mut more: Option<Command> = None;
+        let mut more = false;


There is a compiler warning with this patch sequence that reveals that more is only assigned to, not read. This is vestigial?

I thought I had removed that here, but it turns out it was done elsewhere. Removing...

ncalexan · 2017-05-23T19:45:30Z

+                    .skip(eof())
                    .map(|args| {
                        if args.len() > 0 {
                            return Err(cli::ErrorKind::CommandParse(format!("Unrecognized argument {:?}", args[0])).into());


There's a handy bail! macro specifically to make this type of early return more pleasant.

ncalexan

The commit comment is missing "r" in the "r=nalexander" part.

There is trailing whitespace throughout.

If you are done with this patch, it's good enough to land -- we can revisit the parser as we iterate.

Sorry for the delayed review!

ncalexan · 2017-05-29T22:45:48Z

                        })));

-    let query_parser = try(string(QUERY_COMMAND)).or(try(string(ALT_QUERY_COMMAND)))
+    let edn_arg_parser = || spaces()


I don't see any movement or discussion of #465 (comment) -- did you try and fail to improve this, or do you particularly like the expression as is?

Improving this would avoid .into_iter().collect() two places below, which is a strong sign that your types are wrong: IIUC, you're collecting Vec<String> (where each string is one character!) into a String. That's going to inefficient.

really? I could've sworn I totally rewrote that.

Looking at it, I rewrote it blow here but didn't delete the old one. I think things might've gone wiggy in the rebasing.

ncalexan · 2017-05-29T22:47:25Z

-    let transact_parser = try(string(TRANSACT_COMMAND)).or(try(string(ALT_TRANSACT_COMMAND)))
+    let transact_parser = try(string(LONG_TRANSACT_COMMAND)).or(try(string(SHORT_TRANSACT_COMMAND)))
                    .with(edn_arg_parser())
                    .map( |x| {


nit: no leading space before the argument: .map(|x| { ....

* Bump rust version number. * Use `bail` when throwing errors. * Improve edn parser. * Remove references to unused `more` flag. * Improve naming of query and transact commands.

* Send queries and transactions to mentat and output the results move outputting query and transaction results out of store and into repl * Add query and transact commands to help * Execute queries and transacts passed in at startup * Address review comments =nalexander. * Bump rust version number. * Use `bail` when throwing errors. * Improve edn parser. * Remove references to unused `more` flag. * Improve naming of query and transact commands. * Execute command line args in order * Addressing rebase issues

* Open named database OR default to in memory database if no name provided Rearrange workspace to allow import of mentat crate in cli crate Create store object inside repl when started for connecting to mentat Use provided DB name to open connection in store Accept DB name as command line arg. Open on CLI start Implement '.open' command to open desired DB from inside CLI * Implement Close command to close current DB. * Closes existing open db and opens new in memory db * Review comment: Use `combine` to parse arguments. Move over to using Result rather than enums with err * Accept and parse EDN Query and Transact commands (#453) (#465) * Parse query and transact commands * Implement is_complete for transactions and queries * Improve query parser. Am still not happy with it though. There must be some way that I can retain the eof() after the `then` that means I don't have to move the skip on spaces and eof Make in process command storing clearer. Add comments around in process commands. Add alternative commands for transact/t and query/q * Address review comments r=nalexander. * Bump rust version number. * Use `bail` when throwing errors. * Improve edn parser. * Remove references to unused `more` flag. * Improve naming of query and transact commands. * Send queries and transactions to mentat and output the results (#466) * Send queries and transactions to mentat and output the results move outputting query and transaction results out of store and into repl * Add query and transact commands to help * Execute queries and transacts passed in at startup * Address review comments =nalexander. * Bump rust version number. * Use `bail` when throwing errors. * Improve edn parser. * Remove references to unused `more` flag. * Improve naming of query and transact commands. * Execute command line args in order * Addressing rebase issues

* Create mentat command line. * Create tools directory containing new crate for mentat_cli. * Add simple cli with mentat prompt. * Remove rustc-serialize dependency * Open DB inside CLI (#452) (#463) * Open named database OR default to in memory database if no name provided Rearrange workspace to allow import of mentat crate in cli crate Create store object inside repl when started for connecting to mentat Use provided DB name to open connection in store Accept DB name as command line arg. Open on CLI start Implement '.open' command to open desired DB from inside CLI * Implement Close command to close current DB. * Closes existing open db and opens new in memory db * Review comment: Use `combine` to parse arguments. Move over to using Result rather than enums with err * Accept and parse EDN Query and Transact commands (#453) (#465) * Parse query and transact commands * Implement is_complete for transactions and queries * Improve query parser. Am still not happy with it though. There must be some way that I can retain the eof() after the `then` that means I don't have to move the skip on spaces and eof Make in process command storing clearer. Add comments around in process commands. Add alternative commands for transact/t and query/q * Address review comments r=nalexander. * Bump rust version number. * Use `bail` when throwing errors. * Improve edn parser. * Remove references to unused `more` flag. * Improve naming of query and transact commands. * Send queries and transactions to mentat and output the results (#466) * Send queries and transactions to mentat and output the results move outputting query and transaction results out of store and into repl * Add query and transact commands to help * Execute queries and transacts passed in at startup * Address review comments =nalexander. * Bump rust version number. * Use `bail` when throwing errors. * Improve edn parser. * Remove references to unused `more` flag. * Improve naming of query and transact commands. * Execute command line args in order * Addressing rebase issues * Exit CLI (#457) (#484) r-rnewman * Implement exit command for cli tool * Address review comments r=rnewman * Include exit commands in help * Show schema of current DB (#487) * Fixing rebase issues * addressing nit * Match updated dependencies on CLI crate and remove unused import

* Create mentat command line. * Create tools directory containing new crate for mentat_cli. * Add simple cli with mentat prompt. * Remove rustc-serialize dependency * Open DB inside CLI (mozilla#452) (mozilla#463) * Open named database OR default to in memory database if no name provided Rearrange workspace to allow import of mentat crate in cli crate Create store object inside repl when started for connecting to mentat Use provided DB name to open connection in store Accept DB name as command line arg. Open on CLI start Implement '.open' command to open desired DB from inside CLI * Implement Close command to close current DB. * Closes existing open db and opens new in memory db * Review comment: Use `combine` to parse arguments. Move over to using Result rather than enums with err * Accept and parse EDN Query and Transact commands (mozilla#453) (mozilla#465) * Parse query and transact commands * Implement is_complete for transactions and queries * Improve query parser. Am still not happy with it though. There must be some way that I can retain the eof() after the `then` that means I don't have to move the skip on spaces and eof Make in process command storing clearer. Add comments around in process commands. Add alternative commands for transact/t and query/q * Address review comments r=nalexander. * Bump rust version number. * Use `bail` when throwing errors. * Improve edn parser. * Remove references to unused `more` flag. * Improve naming of query and transact commands. * Send queries and transactions to mentat and output the results (mozilla#466) * Send queries and transactions to mentat and output the results move outputting query and transaction results out of store and into repl * Add query and transact commands to help * Execute queries and transacts passed in at startup * Address review comments =nalexander. * Bump rust version number. * Use `bail` when throwing errors. * Improve edn parser. * Remove references to unused `more` flag. * Improve naming of query and transact commands. * Execute command line args in order * Addressing rebase issues * Exit CLI (mozilla#457) (mozilla#484) r-rnewman * Implement exit command for cli tool * Address review comments r=rnewman * Include exit commands in help * Show schema of current DB (mozilla#487) * Fixing rebase issues * addressing nit * Match updated dependencies on CLI crate and remove unused import

fluffyemily requested a review from ncalexan May 16, 2017 15:58

jaredhirsch added the in progress label May 16, 2017

ncalexan suggested changes May 17, 2017

View reviewed changes

fluffyemily force-pushed the fluffyemily/open-db branch from c0adac7 to b3ff534 Compare May 19, 2017 12:39

Emily Toop added 2 commits May 19, 2017 16:11

Parse query and transact commands

102e528

Implement is_complete for transactions and queries

a8bb996

fluffyemily force-pushed the fluffyemily/parse_edn branch 2 times, most recently from 6489e57 to 874a10a Compare May 22, 2017 12:18

fluffyemily force-pushed the fluffyemily/parse_edn branch from 874a10a to e0548e9 Compare May 22, 2017 14:16

fluffyemily mentioned this pull request May 22, 2017

Send queries and transactions to mentat and output the results #466

Merged

fluffyemily requested a review from ncalexan May 22, 2017 15:58

ncalexan suggested changes May 23, 2017

View reviewed changes

fluffyemily requested a review from ncalexan May 26, 2017 13:45

ncalexan approved these changes May 29, 2017

View reviewed changes

fluffyemily force-pushed the fluffyemily/parse_edn branch 2 times, most recently from 3aa1e5f to 00af587 Compare May 30, 2017 09:58

Emily Toop added 2 commits May 30, 2017 10:58

Address review comments r=nalexander.

00af587

* Bump rust version number. * Use `bail` when throwing errors. * Improve edn parser. * Remove references to unused `more` flag. * Improve naming of query and transact commands.

fluffyemily merged commit abcbdf5 into fluffyemily/open-db May 30, 2017

fluffyemily deleted the fluffyemily/parse_edn branch May 30, 2017 10:05

jaredhirsch removed the in progress label May 30, 2017

Conversation

fluffyemily commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncalexan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncalexan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncalexan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fluffyemily commented May 16, 2017 •

edited

Loading