use simpler GrmtoolsSectionValue throughout - #666
Conversation
| impl<'input> GrmtoolsSectionParser<'input> { | ||
| fn parse_setting(&'_ self, mut i: usize) -> Result<(Setting<Span>, usize), HeaderError<Span>> { | ||
| fn parse_setting( | ||
| &'_ self, |
There was a problem hiding this comment.
Just noticing that now a few of the functions like parse_setting and parse_namespaced which
historically made sense since they previously returned Setting and Namespaced probably need better names like parse_rustlike for parse_namespaced..
Alas I don't have a good idea for parse_setting, which is to the right hand side of the : in value: setting.
| member: ("None".to_string(), from_loc.clone()), | ||
| })), | ||
| RecoveryKind::CPCTPlus => { | ||
| GrmtoolsSectionValue::RustLike("RecoveryKind::CPCTPlus".to_string(), from_loc) |
There was a problem hiding this comment.
I think these can probably be simplified by using the ToTokens/implementations to produce the string
rather than these hard coded values.
Ditto for elsewhere.
There was a problem hiding this comment.
Alas, that doesn't seem to work (it emits a fully qualified path like :: lrpar :: RecoveryKind :: CPCTPlus)
There was a problem hiding this comment.
The best I could come up with to simplify these further was using Debug 3129318
| Flag(bool, T), | ||
| Setting(Setting<T>), | ||
| #[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
| pub enum GrmtoolsSectionValue<T> { |
There was a problem hiding this comment.
I think we can still retain the "The value contained within a Header" comment?
There was a problem hiding this comment.
Perhaps we should also add/retain a comment for the RustLike/Namespaced variant.
There was a problem hiding this comment.
I expanded upon this comment, and added docstrings for Namespaced/RustLike in 02a4bee
| Flag(bool, T), | ||
| Setting(Setting<T>), | ||
| #[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
| pub enum GrmtoolsSectionValue<T> { |
There was a problem hiding this comment.
I wonder if we can find a nicer/shorter name for this. Maybe just HeaderValue or similar?
There was a problem hiding this comment.
I believe HeaderValue is still in use for the HeaderValue(key_span: Span, value: GrmtoolsSectionValue) type, It needed to not be a tuple for orphan rules IIRC/a comment says.
In this patch we deleted a type Value we could just use that perhaps.
| 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.
|
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! |
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?