Simplify Header::Value - #666
Conversation
| mut i: usize, | ||
| ) -> Result<(Namespaced<Span>, usize), HeaderError<Span>> { | ||
| // Either a name alone, or a namespace which will be followed by a member. | ||
| fn parse_rustlike(&self, mut i: usize) -> Result<((String, Span), usize), HeaderError<Span>> { |
There was a problem hiding this comment.
FWIW I marginally prefer "namespaced" as a name ("Rustlike" felt like it could cover a lot of things). I don't feel strongly, though.
There was a problem hiding this comment.
I don't feel strongly about it either, I don't mind switching it back.
There was a problem hiding this comment.
I switched this back to Namespaced in 12efed9
|
It's hard not to like a PR which deletes this much stuff with such a minor impact on functionality! It suggests to me that it's heading in the right direction! |
ratmice
changed the title
Header::Value
| Num(u64, T), | ||
| Bool(bool, T), | ||
| Array(Vec<Value<T>>, T), | ||
| /// A Rust like value, with an optional type namespace. |
There was a problem hiding this comment.
I presume that "optional" here means "non-empty string"?
There was a problem hiding this comment.
Not quite (Edit: I may have misread non-empty as "empty string"), I was about to say I should include an example, but now notice I did, by "optional type namespace" I meant: YaccKind::Grmtools where YaccKind:: is the optional type namespace,
and it's optional because the YaccKind:: can be omitted by just specifying Grmtools.
Maybe I need to specify Grmtools is omitting the optional type namespace in the later example?
There was a problem hiding this comment.
I tried to clarify the comment in a2661d3
There was a problem hiding this comment.
Ah so we expect the optional namespace to end with :: I think?
There was a problem hiding this comment.
Yeah exactly, the actual rule (from grmtools-section.test) is a little bit more complex than just starting with an optional prefix, because it's a recursive rule.
namespaced -> (String, Span): IDENT | IDENT '::' IDENT;
val -> Value<Span>: namespaced | namespaced '(' namespaced ')' | ... ;
In the "Further examples" you can see one of the recursive rules with and without the optional namespace: like Original(UserAction)
|
I have one further, minor, comment, but I think we're close with this one. |
|
I think this is ready for squashing. If you agree, please go ahead. |
This simplifies the `Value` type by making it more json like, It avoids the complexity of exposing the AST form of namespaced rust like values, by converting them back into their string representation.
a2661d3 to
d1af629
Compare
|
Squashed, I also tried to write a higher level overview in the commit message explaining it. |
2 participants
This is an experiment to see if we can do away with
Value<T>and all it's friendsSetting,Namespaced, etc.Replacing it with the much simpler
GrmtoolsSectionValue<T>. This can lead to slightly worse error messages.e.g. because we're checking
YaccKindas a whole includingYaccOriginalActionKind. But I think it is pretty much limited to that. We could do better, by attempting to parse theRustLike(string)rather than the simple matching I've done here. (As well as derive an inner span).Sadly this patch has ended up kind of redoing some of the case-insensitivity migration in #665
This came up because when experimenting with integrading the lookup methods with the used value checking.
This involved changing the value owned by the
GrammarASTfrom aHashMap<GrmtoolsSectionValue>to theHeader<T>. Then the lookup methods would have toclonethe value and return an ownedGrmtoolsSectionValue, because there was none to borrow anymore.It feels like this solves that borrowing issue, but also is a much simpler structure, and so cleans up the code a lot?