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

Remove unused peekable operations from swayfmt #2915

Merged
merged 9 commits into from
Oct 8, 2022
Merged

Conversation

eureka-cpu
Copy link
Contributor

Closes #2496

@eureka-cpu
Copy link
Contributor Author

@mitchmindtree hmm, I know this change was at your request initially but clippy seems to think we should use a for loop 🤔

@mitchmindtree
Copy link
Contributor

@mitchmindtree hmm, I know this change was at your request initially but clippy seems to think we should use a for loop thinking

Ahh, clippy is likely complaining as we're no longer using peak. Are you sure this PR keeps the semantics we're after?

Previously in these loops we were:

  1. Cloning the iterator.
  2. Using the cloned iterator to iterate over the elements.
  3. Calling peak on the old iterator, which meant that only the first element was ever peaked.

The reason I suggested checking if we should be using while let is that step 3 looked suspicious to me. I.e. if we're using peak, the intention was likely to always peak the next element and not repeatedly peak the first element. Does that sound correct?

@eureka-cpu
Copy link
Contributor Author

@mitchmindtree hmm, I know this change was at your request initially but clippy seems to think we should use a for loop thinking

Ahh, clippy is likely complaining as we're no longer using peak. Are you sure this PR keeps the semantics we're after?

Previously in these loops we were:

1. Cloning the iterator.

2. Using the cloned iterator to iterate over the elements.

3. Calling `peak` on the **old** iterator, which meant that _only the first element was ever peaked_.

The reason I suggested checking if we should be using while let is that step 3 looked suspicious to me. I.e. if we're using peak, the intention was likely to always peak the next element and not repeatedly peak the first element. Does that sound correct?

I think I see now, I'll take another look tomorrow morning

@eureka-cpu
Copy link
Contributor Author

eureka-cpu commented Oct 3, 2022

@mitchmindtree is there a reason we wouldn't want to consume the iterator here? Looking again, I removed the .peek() because it removes the last newline which we actually want to keep. Adding the .peek() back in feels sort of redundant because when there isn't another field, we still want to add another newline and comma.

@eureka-cpu
Copy link
Contributor Author

eureka-cpu commented Oct 3, 2022

@mitchmindtree is there a reason we wouldn't want to consume the iterator here? Looking again, I removed the .peek() because it removes the last newline which we actually want to keep. Adding the .peek() back in feels sort of redundant because when there isn't another field, we still want to add another newline and comma.

I went ahead and removed .peekable() from enum, maybe this is the right direction?

@mitchmindtree
Copy link
Contributor

Ahhhh in that case, if there's no need for peek, we can switch back to consuming the iterator with for 👍

Copy link
Contributor

@eightfilms eightfilms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the warning is ignored

@eureka-cpu eureka-cpu changed the title Change for loops to while let in peekable iterations Remove unused peekable operations from swayfmt Oct 7, 2022
@eureka-cpu eureka-cpu enabled auto-merge (squash) October 7, 2022 19:01
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@eureka-cpu eureka-cpu merged commit 61d6d65 into master Oct 8, 2022
@eureka-cpu eureka-cpu deleted the eureka-cpu/2496 branch October 8, 2022 23:43
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.

Use while let in place of for in peekable() iterations in sfv2
5 participants