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 accept when crossing chunk boundary #335

Merged
merged 2 commits into from
Jun 14, 2022
Merged

Fix accept when crossing chunk boundary #335

merged 2 commits into from
Jun 14, 2022

Conversation

satabin
Copy link
Member

@satabin satabin commented Jun 14, 2022

Looping on accept instead of loop restarts reading the string to
accept from the beginning, which results in wrongly reported syntax
error.

Thanks @rossabaker for spotting this bug.

@satabin satabin added bug Something isn't working xml labels Jun 14, 2022
@satabin satabin added this to the 1.4.1 milestone Jun 14, 2022
@satabin satabin requested a review from ybasket June 14, 2022 10:36
Looping on `accept` instead of `loop` restarts reading the string to
accept from the beginning, which results in wrongly reported syntax
error.
@armanbilge
Copy link
Contributor

I believe this was @rossabaker's 🔎 🐛 but 🎉 all the same! :)

@satabin
Copy link
Member Author

satabin commented Jun 14, 2022

I believe this was @rossabaker's mag_right bug but tada all the same! :)

You’re right, sorry for the mis-attribution.

It looks like since upgrading fs2, Scala.JS tests are a bit flaky.
@armanbilge
Copy link
Contributor

It looks like since upgrading fs2, Scala.JS tests are a bit flaky.

Wooops! Can you point me to something specific? I did a big refactor of the internal implementation and probably introduced some bugs 😓

@satabin
Copy link
Member Author

satabin commented Jun 14, 2022

It looks like since upgrading fs2, Scala.JS tests are a bit flaky.

Wooops! Can you point me to something specific? I did a big refactor of the internal implementation and probably introduced some bugs sweat

I was about to ping you 😅
The CI regularly fails like this one. Locally it looks stable, this is weird. I am testing whether this is the update that introduced the problem.

@armanbilge
Copy link
Contributor

Oh dear, thanks, that looks pretty ominous 😱 let's see if I can at least replicate it locally.

@satabin
Copy link
Member Author

satabin commented Jun 14, 2022

@armanbilge it looks like downgrading did the trick. I will release a bugfix release with it. If you need more inputs on my problems, let me know.

@satabin satabin merged commit f5487de into main Jun 14, 2022
@satabin satabin deleted the xml/fix-accept branch June 14, 2022 11:44
@satabin satabin mentioned this pull request Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working xml
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants