softdevteam / grmtools

Rust grammar tool libraries and binaries
Other
507 stars 31 forks source link

Add a Vec<Span> to YaccErrorKind::Duplicate* variants #308

Closed ratmice closed 2 years ago

ratmice commented 2 years ago

It seems like it is best to add other YaccParserErrorKind variants for a nice clean history/merge. (At least as long the variant isn't turning into a any kind of grand adventure).

ltratt commented 2 years ago

Please squash.

ratmice commented 2 years ago

Let me know if you want me to keep adding variants to this series for less merge commits, or if we want to just merge now. Otherwise it is squashed.

ltratt commented 2 years ago

Ah there are other places where we need to do the same thing? Then let's add extra commits here -- it makes sense to do it in one PR.

ratmice commented 2 years ago

Will do, I'll just add a task list to the initial pull-request and perhaps rename the PR.

ltratt commented 2 years ago

Looks good to me. Are we ready for squashing?

ratmice commented 2 years ago

About half-way done with the variants, but I think we are good with these. I wouldn't mind reordering/squashing these commits, then updating the top comment.

ltratt commented 2 years ago

Let's keep going with the commits as-is, then we can do squashing at the end.

ltratt commented 2 years ago

Works for me! Any duplicate-possibilities left?

ratmice commented 2 years ago

Just 3 more, DuplicateStart, DuplicateActionTypeDeclaration, and DuplicateEPP. After that I think it's just lrlex::LexErrorKind::DuplicateName, not sure if we want to handle that here, or in another PR.

Start is taking a bit because of its pervasiveness across the library/testsuite.

ltratt commented 2 years ago

Agreed: lrlex is probably best left to another PR.

ratmice commented 2 years ago

Alright, well that is it I believe. At least I am not aware that the others can return multiple locations, but I haven't done a thorough examination there.

The last one, I ended up adding a second span, just because I thought it would be really confusing if I were to put a tuple of String and Span together if the Span was not actually for the String, but the key.

Edit: There are a few issues in the commit messages, So i'll edit those when the time comes.

ltratt commented 2 years ago

Please squash.

ratmice commented 2 years ago

Squashed, but do let me know if you prefer it squashed further into one commit. Edit: and I'm super confused, because these 11 commits don't look at all like the 8 commits i'm staring at locally.

ltratt commented 2 years ago

It looks like they're correct now!

bors r+

ratmice commented 2 years ago

My fault pushed the wrong branch accidentally, fixed.

bors[bot] commented 2 years ago

Build succeeded: