Use mentat-parser-utils in tx-parser. Initial stab at #235 #249

joewalker · 2017-02-06T16:35:22Z

Work in progress patch just to see if I'm on the right lines.

victorporof · 2017-02-06T16:37:01Z

tx-parser/src/lib.rs


 use combine::{any, eof, many, optional, parser, satisfy_map, token, Parser, ParseResult, Stream};
 use combine::combinator::{Expected, FnParser};
 use edn::symbols::NamespacedKeyword;
 use edn::types::Value;
-use mentat_tx::entities::*;
+use mentat_tx::entities::{Entid, EntidOrLookupRef, Entity, LookupRef, ValueOrLookupRef};


Any reason to import stuff explicitly instead of everything?

It's good style. Using * means that a simple addition in a different crate can cause this one to fail to compile.

There's a clash of styles it would seem, as @ncalexan opted for * in the first place. Have we settled on a single style for our codebase? My question was directed to the fact that I didn't see how it this change was related to the issue being fixed (it could've been for what I know, I still don't have a solid grasp on rust).

I changed it because I misread an error message telling me it wasn't needed (it was actually the one down around line 274 in mod tests).

I agree with @rnewman's logic, and I'd also add that some specificity about what's required and what isn't is a good thing from a purely visual POV to humans too

victorporof · 2017-02-06T16:40:51Z

query-parser/src/parser_util.rs

@@ -40,6 +40,7 @@ macro_rules! matches_plain_symbol {
 /// The provided `$body` will be evaluated with `$input` bound to the input stream.
 ///
 /// `$body`, when run, should return a `ParseResult` of the appropriate result type.
+#[macro_export]


Should probably go ahead and export all of these macros.

Nothing in query-parser should be exported; if it's directly applicable to other crates, it should be moved to parser-utils.

I thought not putting these in the parser_utils crate in the first place was intentional. If moved there though, they should indeed be exported as well.

It was: they're very EDN-specific, and I wasn't sure how reusable they would be.

It looks like @joewalker could directly use def_value_satisfy_parser_fn!, though…

So I'm moving all (?) the parser utils from parser_utils to parser-utils, right?

My understanding is: if it can be used in other places as well, then yes. However, indeed in this particular case def_value_satisfy_parser_fn could be used instead of def_parser_fn, so maybe don't move unless there's a really good reason to?

The macros are built on each other, so you can't move and use d_v_s_p_f without moving the others.

@victorporof You're saying we could use def_value_satisfy_parser_fn here? I'm not sure how - what should the path be?

def_value_satisfy_parser_fn!(Tx, integer, i64, Value::Integer(x));

Doesn't work

victorporof · 2017-02-06T16:42:17Z

tx-parser/src/lib.rs

@@ -13,12 +13,17 @@
 extern crate edn;
 extern crate combine;
 extern crate mentat_tx;
+extern crate mentat_parser_utils;


This is also likely to require a macro_use pragma if you plan on using the macros there. I'd be surprised if not.

victorporof · 2017-02-06T16:44:05Z

tx-parser/src/lib.rs

@@ -30,9 +35,19 @@ fn fn_parser<O, I>(f: fn(I) -> ParseResult<O, I>, err: &'static str) -> TxParser
    parser(f).expected(err)
 }

+def_parser_fn!(Tx, integer, Value, i64, input, {


Should probably not have to define macros here. There's another def_parser_fn! as well in the mentat query parser crate. Did you intend to redefine it here? I'm having a hard time understanding what this is for as well, since it's not used anywhere.

def_parser_fn! is a macro invocation, not a macro definition.

Ah! I misread this!

ncalexan · 2017-02-06T20:40:25Z

There's a clash of styles it would seem, as @ncalexan opted for * in the first place. Have we settled on a single style for our codebase? My question was directed to the fact that I didn't see how it this change was related to the issue being fixed (it could've been for what I know, I still don't have a solid grasp on rust).

Yeah, I think this was a mistake. In some cases it makes sense -- for example, during development -- but in most cases it's best to just list them. Filed #251 to unify on the non-use ... * form throughout the code base.

See mozilla#249 (comment)

victorporof · 2017-02-08T11:29:37Z

@joewalker Is this still WIP?

joewalker · 2017-02-09T11:28:21Z

Is this still WIP?

Erm, maybe?

Do we have any test coverage info anywhere? All the tests pass, but can I be sure we're testing it properly?
Are there other places where we should do this?
I'm still not very confident that I did it correctly TBH.

joewalker · 2017-02-09T11:40:10Z

Something that makes me think I'm not done yet - The pre-PR version of entities contains "[:db/add|:db/retract ...]", but the current post PR version doesn't contain this anywhere.

joewalker · 2017-02-09T15:16:15Z

Elsewhere @rnewman said (of lib.rs:48)

This can be phrased in terms of def_value_satisfy_parser_fn!.

Which sounds likely, except that I'm stuck on what $transformer should be. I'd like to say:

def_value_satisfy_parser_fn!(Tx, integer, i64, if let Value::Integer(y) = x {
        Some(y)
    } else {
        None
    });

But nope. Any ideas?

joewalker · 2017-02-09T15:21:21Z

About to squash and rebase. Head was at 12f17c1

rnewman · 2017-02-09T16:52:54Z

tx-parser/src/lib.rs

-            .parse_stream(input);
-    }
+// The above using a def_value_satisfy_parser_fn
+//def_value_satisfy_parser_fn!(Tx, integer, i64, if let Value::Integer(y) = x { Some(y) } else { None });


In practice this allows you to simply pass a function that accepts an &edn::Value and
returns an Option<$result_type>: if a suitable value is at the front of the stream,
it will be converted and returned by the parser; otherwise, the parse will fail.

So if you define a function:

fn value_to_i64(val: &Value) -> Option<i64> { if let &Value::Integer(i) = val { Some(i) } else { None } }

you can use it here:

def_value_satisfy_parser_fn!(Tx, integer, i64, value_to_i64);

Now it turns out we already have that function: it's Value::as_integer. So this should work:

def_value_satisfy_parser_fn!(Tx, integer, i64, Value::as_integer);

OK, except that the signature of as_integer is (&self) -> Option<&i64>, and we want -> Option<i64>. So maybe I just include the function as stated?

i64 implements Copy, so we should fix the definition of as_integer (and as_float, as_boolean). No sense returning a reference type when the raw type is no larger than a machine address!

I can do that very quickly in a side PR. Momento…

#269, @joewalker.

rnewman · 2017-02-09T16:54:39Z

tx-parser/src/lib.rs

+// Manually expanding the above:
+// def_value_parser_fn!(Tx, integer, i64, input, {
+//     satisfy_map(|x: edn::Value| $transformer(&x)                                      ).parse_stream(input)
+//     satisfy_map(|x: edn::Value| if let Value::Integer(y) = x { Some(y) } else { None }).parse_stream(input)


The trick is that $transformer is a path, not a block, so you can't pass an expression to this macro.

rnewman · 2017-02-09T17:02:01Z

tx-parser/src/lib.rs

-            .parse_stream(input);
-    }
+def_parser_fn!(Tx, keyword, Value, NamespacedKeyword, input, {
+    return satisfy_map(|x: Value| if let Value::NamespacedKeyword(y) = x {


Similarly, this would be Value::as_namespaced_keyword.

Oh, and if this can't be done, because you get back a ref, then we perhaps want Value::to_namespaced_keyword, which would simply be

pub fn to_namespaced_keyword(&self) -> Option<NamespacedKeyword> { self.as_namespaced_keyword().map(|x| x.clone()) }

Didn't see this comment until I'd worked it out myself, except I wrote:

fn value_to_namespaced_keyword(val: &Value) -> Option<NamespacedKeyword> { if let &Value::NamespacedKeyword(ref i) = val { Some(i.clone()) } else { None } }

I think it makes sense to do this in Value though.

I did this using macros: joewalker@a4ba10c

rnewman · 2017-02-09T17:03:05Z

tx-parser/src/lib.rs

-                match r {
+// TODO: abstract the "match Vector, parse internal stream" pattern to remove this boilerplate.
+def_parser_fn!(Tx, add, Value, Entity, input, {
+    return satisfy_map(|x: Value| -> Option<Entity> {


You can omit the return and the closing semicolon, saving yourself some space.

joewalker · 2017-02-10T12:57:09Z

I think we can land this. OK?

rnewman · 2017-02-10T17:27:22Z

tx-parser/src/lib.rs

-        return satisfy_map(|x: Value| if let Value::Integer(y) = x {
-                Some(y)
+fn value_to_namespaced_keyword(val: &Value) -> Option<NamespacedKeyword> {
+    if let &Value::NamespacedKeyword(ref i) = val { Some(i.clone()) } else { None }


I think you can use val.as_namespaced_keyword().map(|x| x.clone()) here.

Or even:

def_value_satisfy_parser_fn!(Tx, keyword, NamespacedKeyword, Value::into_namespaced_keyword);

Since the landing of 49f91b0

Sigh. Nope. I'll do it your way and land it, and work out the fancy thing later.

…ctorporof Move macros query-parser/…/parser_utils.rs → parser-utils/…/query.rs Signed-off-by: Joe Walker <jwalker@mozilla.com>

joewalker assigned victorporof Feb 6, 2017

joewalker added the in progress label Feb 6, 2017

victorporof reviewed Feb 6, 2017

View reviewed changes

ncalexan mentioned this pull request Feb 6, 2017

Replace use ... * with explicit lists #251

Closed

joewalker added a commit to joewalker/mentat that referenced this pull request Feb 7, 2017

Move macros query-parser/…/parser_utils.rs → parser-utils/…/query.rs

86dc118

See mozilla#249 (comment)

joewalker mentioned this pull request Feb 9, 2017

Initial stab at #251 #252

Closed

joewalker force-pushed the use-macro branch from 12f17c1 to cf19b4f Compare February 9, 2017 15:47

rnewman reviewed Feb 9, 2017

View reviewed changes

joewalker force-pushed the use-macro branch from cf19b4f to 3f5b896 Compare February 10, 2017 12:29

joewalker assigned joewalker and unassigned victorporof Feb 10, 2017

rnewman approved these changes Feb 10, 2017

View reviewed changes

Use mentat-parser-utils in tx-parser. Fixes mozilla#235; r=rnewman,vi…

f591c90

…ctorporof Move macros query-parser/…/parser_utils.rs → parser-utils/…/query.rs Signed-off-by: Joe Walker <jwalker@mozilla.com>

joewalker force-pushed the use-macro branch from 3f5b896 to f591c90 Compare February 10, 2017 18:30

joewalker merged commit f591c90 into mozilla:rust Feb 10, 2017

joewalker removed the in progress label Feb 10, 2017

joewalker deleted the use-macro branch February 10, 2017 18:34

Use mentat-parser-utils in tx-parser. Initial stab at #235 #249

Use mentat-parser-utils in tx-parser. Initial stab at #235 #249

Conversation

joewalker commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joewalker Feb 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorporof Feb 6, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncalexan commented Feb 6, 2017

victorporof commented Feb 8, 2017

joewalker commented Feb 9, 2017

joewalker commented Feb 9, 2017

joewalker commented Feb 9, 2017

joewalker commented Feb 9, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joewalker commented Feb 10, 2017

Choose a reason for hiding this comment

joewalker Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joewalker Feb 6, 2017 •

edited

Loading

victorporof Feb 6, 2017 •

edited

Loading

joewalker commented Feb 9, 2017 •

edited

Loading

joewalker Feb 10, 2017 •

edited

Loading