Skip to content

refac: replace MultiPeek by Peekable #5579

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

Closed

Conversation

marcospb19
Copy link

MultiPeek is only useful in cases when it's necessary
to peek multiple items ahead.

@ytmimi
Copy link
Contributor

ytmimi commented Oct 31, 2022

Thanks for your first contribution to rustfmt 🎉

It looks like MultiPeek was added in this PR #2527, so I'd want to double check that we don't regress into the issue that it was trying to solve for.

I also want to note that I've been working on a fix for #4879 on my fork of rustfmt here, and I use MultiPeek to look ahead several characters.

@marcospb19
Copy link
Author

Thanks for your first contribution to rustfmt tada

Thanks!

Sorry, I re-checked MultiPeek implementation and I can confirm this DOES break the function is_raw_string_suffix.

I will turn this PR into a draft and I'll see if I can add a test to check this behavior, and compare the two codes with it.

@marcospb19 marcospb19 marked this pull request as draft November 1, 2022 00:00
@ytmimi
Copy link
Contributor

ytmimi commented Nov 1, 2022

Great! Keep me posted on how the investigation goes!

@marcospb19
Copy link
Author

@ytmimi my investigation was inconclusive, a test was added to check for this:

https://github.com/rust-lang/rustfmt/blob/master/tests/target/issue-2526.rs

However, the test has not failed after I replaced MultiPeek by Peekable, which I do not fully understand, maybe it does not break things, but anyways, I think I prefer not to touch on this piece of code.

Let me know if you have any thoughts on this tho.

@marcospb19 marcospb19 closed this Nov 9, 2022
@ytmimi
Copy link
Contributor

ytmimi commented Nov 10, 2022

Unfortunately I'd need to dive into this myself to understand what's going on here. I'm happy to closing this PR as there isn't an immediate need to change this part of the codebase.

If you've still got some interest in contributing to rustfmt I'd encourage you to take a look at the backlog and see if any open issue catches your attention. If you need any help don't hesitate to reach out 😁

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

Successfully merging this pull request may close these issues.

2 participants