Add line/column info to nimbleparses printing of HeaderError.
#564
Conversation
|
One thing I notice in the output is that the first instance doesn't have a I am also wondering whether you think it is worth trying larger follow up where we try moving this diagnostics formatting code to a This would be a different approach where since it doesn't try to add any API where people can customize error printing, but just shares the nimbleparse error printing code between the aformentioned things? |
My gut reaction is to first see what we can break into a separate module within nimbleparse; and, depending on how that goes, maybe we can then put that in another crate. Warning: this might not be a good idea on my part! |
The It seemed like something I haven't tried yet at least |
|
Please squash. |
`occupied_entry()` is documented as being for a specific use case of mutating the flags for the *key*, using `occupied_entry()` with `insert` runs afoul of this by assuming it can also return a value previously associated with the key. This is a case where the header's valueless key API doesn't coincide very well with the existing `std::collections` entry API. In a sense `Header` can be occupied with a key *without* any associated value. While for the `std::collections` entry API the existence of a key implies the existence of a value. This fixes the problematic usage of the API, but it would be good to revisit the non_standard `occupied_entry()` function, and try to make another API that is more misuse resistent such as perhaps `key_entry` which allows you to modify the flags associated with a key, but doesn't assume the existence of a value.
Move lazy initialisation to using `OnceCell` inside the formatter. Restore some spanned error printing that was lost in commit 277ae03 when `Location` was still within `YaccGrammarError`.
ratmice
force-pushed
the
spanned_header_errors
branch
from
c6605c4 to
6912702
Compare
|
Squashed, but I left a long commit message for be35407 describing that the API is somewhat prone to misuse and perhaps some future changes that could be made to avoid this kind of error in the future. I'm not sure it is imperative that we fix that immediately, considering it's hidden API and the only other usage of the API looks fine. |
| @@ -141,12 +141,12 @@ fn main() { | |||
| }; | |||
| let entry = match header.entry("yacckind".to_string()) { | |||
| Entry::Occupied(_) => unreachable!("Header should be empty"), | |||
| Entry::Vacant(v) => v.occupied_entry(), | |||
| Entry::Vacant(v) => v, | |||
There was a problem hiding this comment.
I think rather than what I discussed in the commit message, of adding a different key_entry, it's best to just implement the mark_required and set_merge_behavior functions for VacantEntry directly, perhaps mark_used too but I can't think of a reason to mark a vacant entry used.
I still think it's best to do as a separate patch, but let me know if you feel otherwise.
In the process replace some usage of
OptionwithOnceCell.It also restores some spanned error printing that was lost in commit 277ae03 when
Locationwas inYaccGrammarError.This is a follow up to PR #561 in that PR the errors when run through nimbleparse looked like:
After this patch they include source context information,