Open DB inside CLI (#452) by fluffyemily · Pull Request #463 · mozilla/mentat

fluffyemily · 2017-05-10T09:44:24Z

Pass in db as command line arg
Open from inside repl using .open [db_name]
Default to in memory DB if no db arg provided.

rnewman · 2017-05-10T12:14:25Z

+static COMMANDS: &'static [CommandDef] = &[
+    CommandDef{name: "open", args: Some("<database>"),
+        accepts: CmdArgs::Text,
+        help: "Open a database with the provided name. Creates a new database of that name if it doesn't already exist."},


Is this a path, a URI, or a name?

Should we also add close?

rnewman · 2017-05-10T12:17:30Z

+];
+
+/// Looks up a command name by what may be an abbreviated prefix.
+/// Returns the `CommandDef` structure if one is found.


This is a hacky way of doing what getopts calls abbreviation to uniqueness. It'll only work so long as none of your commands share a prefix — close and create, for example, will clash for .c.

Think about whether you need to do this.

rnewman · 2017-05-10T12:20:57Z

+/// Looks up a command name by what may be an abbreviated prefix.
+/// Returns the `CommandDef` structure if one is found.
+pub fn lookup_command(name: &str) -> Option<&'static CommandDef> {
+    for cmd in COMMANDS.iter() {


Use .find instead of iterating manually. This becomes a one-liner.

rnewman · 2017-05-10T12:21:41Z

+}
+
+/// Calls the given closure for each command whose name begins with `prefix`.
+pub fn search_command<F>(prefix: &str, mut f: F)


Delete dis.

rnewman · 2017-05-10T12:25:41Z

-                }
+                },
+                InputError(err) => {
+                    if let Some(err) = err {


When will we not have an error in the input error? In that case, can we have a different enum?

rnewman · 2017-05-10T12:26:56Z

    fn handle_command(&mut self, cmd: String, args: Option<String>) {
-        println!("{:?} {:?}", cmd, args);
+        match lookup_command(&cmd).map(|c| c.name) {
+            Some("open") => {


Look up map_or and unwrap_or and friends.

rnewman · 2017-05-15T13:20:36Z

+        match self {
+            &Command::Query(_) |
+            &Command::Transact(_) => false,
+            _ => true


Enumerate every command rather than using _ — that way this'll fail to compile if you add a new command and forget to extend this match!

rnewman · 2017-05-15T13:22:40Z

+        Command::Help(args)
+    });
+
+    let open_parser = string(OPEN_COMMAND).and(space()).and(many1::<Vec<_>, _>(alpha_num()).and(eof())).map(|x| {


Shouldn't this be skip_many(space())?

For a file path you'll need more than alpha_num:

mentat=> .open /tmp/foo.db Invalid command ".open /tmp/foo.db" mentat=> .open foo.db Invalid command ".open foo.db"

rnewman · 2017-05-15T13:23:48Z

+    .and(choice::<[&mut Parser<Input = _, Output = Command>; 3], _>
+        ([&mut try(help_parser),
+          &mut try(open_parser),
+          &mut try(close_parser),]))


If you put .skip(eof()) on the end here, you can remove it from the parsers above.

rnewman · 2017-05-15T13:27:22Z

        match self.reader {
            Some(ref mut r) => {
                r.set_prompt(prompt);
                r.read_line().ok().and_then(|line| line)


Why do we need that and_then?

rnewman · 2017-05-15T13:28:29Z

+}
+
+fn db_output_name(db_name: &String) -> String {
+    if db_name.is_empty() { "in memory db".to_string() } else { db_name.clone() }


rnewman · 2017-05-15T13:30:10Z

+// CONDITIONS OF ANY KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations under the License.
+
+use combine::{eof, many, many1, sep_by, skip_many, token, Parser};


many is unused.

rnewman · 2017-05-15T13:30:58Z

+
+    let close_parser = string(CLOSE_COMMAND).and(skip_many(space())).and(eof()).map( |_| Command::Close );
+
+    token('.')


Allow leading whitespace?

mentat=> .help Invalid command " .help"

rnewman · 2017-05-15T13:32:20Z

+            }
+        } else {
+            for arg in args {
+                let msg = COMMAND_HELP.get(arg.as_str());


You should allow the command arg to start with a . — I immediately typed .help .open and got all of the commands, 'cos it wanted .help open.

rnewman · 2017-05-15T13:35:24Z

+            Command::Open(db) => {
+                self.store.open(Some(db));
+            },
+            Command::Close => self.store.close(),


mentat=> .open foo Database "foo" opened mentat=> .close Database "foo" closed mentat=> .close Database "in memory db" closed mentat=> .close Database "in memory db" closed

I think we should consider having quit == close, and open = close+open, just like sqlite3.

so to close the existing db and open a new in-memory DB would be

mentat=> .open

to close existing db and exit would be

mentat=> .quit

?

ncalexan

I suggested some parsing changes and some changes to the types, specifically around using Result. Feel free to counter; I haven't read your interactions with @rnewman, which would probably cover this.

For the next review round, let's fold this down to 1-2 commits. The iteration of the change sequence is not helping review.

ncalexan · 2017-05-16T20:48:06Z


 [workspace]
-members = []
+members = ["tools/cli"]


Hmm, I had to go read https://github.com/nox/rust-rfcs/blob/master/text/1525-cargo-workspace.md to understand this. Should we be using workspaces more generally? If I understand correctly, we could make most of the existing crates share a workspace, avoiding to rebuild shared dependencies.

ncalexan · 2017-05-16T20:48:52Z

+// CONDITIONS OF ANY KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations under the License.
+
+use combine::{any, eof, many, many1, sep_by, skip_many, token, Parser};


We settled on the

use very::{ long, vertical, list, style, };

ncalexan · 2017-05-16T20:58:45Z

+
+pub fn command(s: &str) -> Command {
+    let help_parser = string(HELP_COMMAND)
+                    .and(skip_many(space()))


.with and .skip let you drop leading and trailing input, respectively. So this is equivalent to

string("help").with(skip_many(space())).with(many::<Vec<_>, _> ...

Then you won't need to index in the way you are.

ncalexan · 2017-05-16T23:40:19Z

+    let help_parser = string(HELP_COMMAND)
+                    .and(skip_many(space()))
+                    .and(many::<Vec<_>, _>(try(any())))
+                    .map(|x| {


So, you're parsing whitespace separated strings here; why not use combine to do this? I'll push a patch to show what I mean.

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

+    Help(Vec<String>),
+    Open(String),
+    Close,
+    Err(String),


You've encoded errors into your Command type, which seems unnatural to me. Why not use Result<Command, AppropriateErrorType> as appropriate?

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

+}
+
+impl Command {
+    pub fn is_complete(&self) -> bool {


A comment explaining what "complete" means would help.

ncalexan · 2017-05-16T23:44:02Z

+        }
+    }
+
+    #[test]


Great tests!

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

+        }
+    }
+
+    fn help_command(&self, args: Vec<String>) {


Github wouldn't let me comment on the tools/cli/src/mentat_cli/store.rs file, so here it is:

I expect we will want to grow this into something (like Conn) in core, so rather than expect() and swallowing the underlying error, can we return Result<Store, AppropriateErrorType> and handle errors like this at a higher level?

If you really want Store to be CLI specific and to handle errors inline, I think you need to surface the underlying error in some way.

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

* Closes existing open db and opens new in memory db

Move over to using Result rather than enums with err

* 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

* 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 rnewman May 10, 2017 09:44

jaredhirsch added the in progress label May 10, 2017

rnewman reviewed May 10, 2017

View reviewed changes

rnewman reviewed May 15, 2017

View reviewed changes

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

ncalexan reviewed May 16, 2017

View reviewed changes

Emily Toop and others added 3 commits May 19, 2017 13:19

Implement Close command to close current DB.

45d00c4

* Closes existing open db and opens new in memory db

Review comment: Use combine to parse arguments.

b3ff534

Move over to using Result rather than enums with err

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

fluffyemily mentioned this pull request May 22, 2017

Accept and parse EDN Query and Transact commands (#453) #465

Merged

fluffyemily merged commit 46fc161 into fluffyemily/cli May 30, 2017

jaredhirsch removed the in progress label May 30, 2017

fluffyemily deleted the fluffyemily/open-db branch May 30, 2017 10:06


		let close_parser = string(CLOSE_COMMAND).and(skip_many(space())).and(eof()).map( \|_\| Command::Close );

		token('.')

+                      }
+                  }
+                  #[test]

Conversation

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

Uh oh!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fluffyemily commented May 10, 2017 •

edited

Loading