Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_formatter): Fill fits #3374

Merged
merged 4 commits into from
Oct 10, 2022
Merged

fix(rome_formatter): Fill fits #3374

merged 4 commits into from
Oct 10, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Oct 10, 2022

Summary

Implements the fix for #3251 (comment)

This PR addresses an issue in the Printer where fill printed the item in flat mode even though the separator neither fits in flat nor expanded mode. In that case, it is necessary to expand the item to avoid overflowing the configured print width.

Test Plan

See the updated snapshots.

@netlify
Copy link

netlify bot commented Oct 10, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit e69ada9
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/6343e4274b57c900086638c4

@MichaReiser MichaReiser temporarily deployed to netlify-playground October 10, 2022 06:54 Inactive
@github-actions
Copy link

github-actions bot commented Oct 10, 2022

@MichaReiser MichaReiser temporarily deployed to netlify-playground October 10, 2022 08:27 Inactive
@@ -37,9 +37,6 @@ pub(super) struct StackedStack<'a, T> {
/// The content of the original stack.
original: &'a [T],

/// The index of the "top" element in the original stack.
original_top: usize,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that having another pointer to point to the "top" of the original stack isn't really necessary, considering that a slice already has two pointers:

  • one pointing to the start
  • one pointing to the end (top)

This change updates the slice inside of pop to remove the need for an extra original_top index.

Ok(Fits::Maybe)
}
/// Tests if the passed element fits on the current line or not.
fn fits_element(&mut self, element: &'a FormatElement) -> PrintResult<Fits> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only changes to this method are that I had to rewrite state to self.state, queue to self.queue, ... plus the change in indention level.

@MichaReiser MichaReiser temporarily deployed to netlify-playground October 10, 2022 08:38 Inactive
@MichaReiser MichaReiser added this to the 10.0.0 milestone Oct 10, 2022
@MichaReiser MichaReiser temporarily deployed to netlify-playground October 10, 2022 09:21 Inactive
@rome rome deleted a comment from github-actions bot Oct 10, 2022
@MichaReiser MichaReiser marked this pull request as ready for review October 10, 2022 09:22
@MichaReiser MichaReiser requested a review from a team October 10, 2022 09:22
@MichaReiser MichaReiser merged commit 9ef58af into main Oct 10, 2022
@MichaReiser MichaReiser deleted the fix/fill-fits branch October 10, 2022 13:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants