Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix line positions #555

Merged
merged 3 commits into from
Feb 9, 2022
Merged

Fix line positions #555

merged 3 commits into from
Feb 9, 2022

Conversation

JonasAlaif
Copy link
Contributor

No description provided.

@JonasAlaif
Copy link
Contributor Author

We should also add a CI test for this

@marcoeilers
Copy link
Contributor

Looks good to me.
That was quick, nice!

@JonasAlaif
Copy link
Contributor Author

JonasAlaif commented Feb 9, 2022

@marcoeilers there is just one thing that I'm not sure about; there was a comment:

// As opposed to use Index ~ t ~ Index, this implementation is agnostic to white space specializations.

My solution is to pretty much use Index ~~ t ~~ Index (except that using exactly that didn't work for some reason) - do you know what @fabiopakk mean by this? Will we run into issues with this new version if we encounter "white space specializations" whatever that is?

@marcoeilers
Copy link
Contributor

No, I don't really remember how the whitespace handling works at all. I guess you can have different parse rules being either whitespace agnostic or not, but I don't know who's specializing what where.

But I tested the code on an example that had all the whitespace-related cases I could think of and it always worked, so whatever it's doing it seems to be right.

@JonasAlaif
Copy link
Contributor Author

JonasAlaif commented Feb 9, 2022

The key difference is between ~ and ~~ which are used to join two parsers; for instance using the former "hello" ~ "world" would match any of "helloworld", "hello world", ..., whereas the latter "hello" ~~ "world" would only match "helloworld". I guess the concern was that with Index ~ t ~ Index the first Index could take the position at an arbitrary number of whitespaces before, not actually giving you the start of the t itself? In that case using Index ~~ t ~~ Index instead, as I do, would alleviate this concern.

@JonasAlaif JonasAlaif merged commit 014db3d into master Feb 9, 2022
@JonasAlaif JonasAlaif deleted the fix-line-positions branch February 9, 2022 13:06
@marcoeilers
Copy link
Contributor

Yeah, makes sense.

@ArquintL
Copy link
Member

ArquintL commented Feb 9, 2022

@JonasAlaif Awesome, thanks a lot for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants