Have the EDN parser store keywords and symbols as rich types. Fixes #154. by rnewman · Pull Request #163 · mozilla/mentat

rnewman · 2017-01-11T22:53:58Z

This demonstrates some unpleasantness with name reuse between enums and structs; the verbosity encourages the use of a test helper function.

Rust also doesn't infer types as much as I'd like, so you can't, say:

let v = keyword(":foo/bar");     // Parser function: returns a Value, specifically Value::Keyword
assert_eq!(v.unwrap().0, keyword::Keyword::namespaced("foo", "bar"));

because Rust doesn't know which enum value v will be.

I could 'inline' keyword::Keyword into Value… but then you can't define functions that work only on keywords; they'd all have to accept a Value and handle the other cases.

ncalexan

Bombs away for commits 1 and 3; I think a little time will improve commit 2 significantly.

ncalexan · 2017-01-11T22:58:00Z

+namespace_separator = "/"
 keyword_char_initial = ":"
+
 // TODO: More chars here?


Is this comment still relevant? _, *, and - seem like good name{space} chars...

I decided not to attack that yet. Start narrow and broaden later.

ncalexan · 2017-01-11T22:58:38Z

-}
+keyword -> Value
+    = keyword_char_initial ns:( kns:$(keyword_namespace) namespace_separator { kns })? n:$(keyword_name) {
+        Value::Keyword(keyword::Keyword::perhaps_namespaced(ns, n))


Is this a place where From/Into can help?

ncalexan · 2017-01-11T23:01:19Z

    Text(String),
    Symbol(String),
-    Keyword(String),
+    Keyword(keyword::Keyword),


Yeah, I really think we want impl From<Into<String>> for keyword::Keyword ... if that's legal, or From<String> if not. Then we can still construct Value::Keyword the easy way.

Two reasons why not:

Not all strings are valid keywords.

Most constructions are namespaced, which makes that approach confusing.

The latter "namespaced" refers to Rust's namespacing, not EDN's, right? I think you could make impl TryFrom<Into<String>> ... and Keyword(":foo/bar") work.

No, namespaced is the specific constructor function for :foo/bar as opposed to :bar.

Keywords are essentially in two disjoint sets: those with namespaces and those without.

I modeled this as an Option<String> for the namespace, because I didn't want yet another layer of enums. I modeled this as two constructor functions with one helper (perhaps_namespaced) to avoid passing None in places.

This is a spot where you can use some trait tricks to allow coercion from &str to Some<&str> to Some<String> within the same signature, but that would make the constructors relatively more complicated.

ncalexan · 2017-01-11T23:01:54Z

 use edn::parse::*;

+// Helper for making wrapped keywords with a namespace.
+fn k_ns(ns: &str, name: &str) -> Value {


Let's spend some time to learn Into and From; these are solved problems in Rust-land.

Maybe not worth doing the namespace bit in the parser, but instead doing it in the constructor. Since you have to check for empty strings anyway, might as well have one place that encodes the rules.

We can't use From: those conversions cannot fail, and not all strings can be turned into a keyword.

Indeed, even TryFrom returns a Result, and thus complicates matters.

This is the root of parsing, and why I picked this division: the struct and its constructors aren't failable (they include panicking validation, but that's another matter), whereas the parser itself will refuse to parse a malformed keyword.

OK, this is fine for now.

joewalker · 2017-01-12T09:56:50Z


 use std::collections::{BTreeSet, BTreeMap, LinkedList};
 use std::iter::FromIterator;
+


Something I miss from Java was a rough standard of individual imports, sorted alphabetically, inserted by some refactoring and automatically folded by the editor. Minimal code churn which the developer didn't need to think about.
I suspect that rust tooling will catch up shortly. So nothing to do now, mostly thinking aloud.

Yeah, I decided to do much what we've been doing in Swift and Java: split std out, sort all alphabetically within that division.

rnewman · 2017-01-12T17:24:24Z

a152e60

rnewman self-assigned this Jan 11, 2017

rnewman requested a review from ncalexan January 11, 2017 22:53

rnewman added the in progress label Jan 11, 2017

ncalexan reviewed Jan 11, 2017

View reviewed changes

rnewman force-pushed the rnewman/edn-keyword branch from f4e0860 to 1025fe7 Compare January 12, 2017 01:11

rnewman changed the title ~~Have the EDN parser store keywords as a rich type. Partially fixes #154.~~ Have the EDN parser store keywords and symbols as rich types. Fixes #154. Jan 12, 2017

Read EDN keywords and symbols as rich types. Fixes #154. r=nalexander

7ff0acc

rnewman force-pushed the rnewman/edn-keyword branch from 1025fe7 to 7ff0acc Compare January 12, 2017 01:18

This was referenced Jan 12, 2017

Add tx{-parser} crates; start parsing transactions. #164

Merged

Make the EDN parser support the correct tokens for Keyword and Symbol #154

Closed

joewalker reviewed Jan 12, 2017

View reviewed changes

rnewman closed this Jan 12, 2017

rnewman deleted the rnewman/edn-keyword branch January 12, 2017 17:24

rnewman removed the in progress label Jan 12, 2017


		use std::collections::{BTreeSet, BTreeMap, LinkedList};
		use std::iter::FromIterator;

Conversation

rnewman commented Jan 11, 2017

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!

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!

rnewman commented Jan 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants