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

feature(rome_js_formatter): Inspect memoized content #2772

Merged
merged 5 commits into from
Jun 24, 2022

Conversation

MichaReiser
Copy link
Contributor

Summary

This PR adds the inspect method to Memoized that allows inspecting the formatted content.

This can be useful in scenarios where some formatting must determine if some content will break to decide the best formatting.

This is different from #2771 because the buffer should be preferred in cases where the fact whatever some content breaks is only used to determine the formatting of another element because it avoids interning/memoizing the inner content.

The API shown here should be preferred when the formatting of the element itself depends on whatever its content breaks where using the WillBreakBuffer would need to rewind to the previous state.

Test Plan

I added a new doc test showing how to use the new API

@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 07:53 Inactive
@cloudflare-pages
Copy link

cloudflare-pages bot commented Jun 24, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4133391
Status: ✅  Deploy successful!
Preview URL: https://f69a4a8b.tools-8rn.pages.dev
Branch Preview URL: https://feature-inspect-memoized.tools-8rn.pages.dev

View logs

@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 07:54 Inactive
@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 07:55 Inactive
@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 07:56 Inactive
@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 07:56 Inactive
@github-actions
Copy link

github-actions bot commented Jun 24, 2022

@MichaReiser MichaReiser changed the title Feature/inspect memoized feature(rome_js_formatter): Inspect memoized content Jun 24, 2022
crates/rome_formatter/src/format_extensions.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 09:43 Inactive
@MichaReiser
Copy link
Contributor Author

@ematipico please merge the PR if you are happy with it as I'll be on PTO.

@MichaReiser
Copy link
Contributor Author

@denbezrukov this or #2771 might be useful for you when testing if an element has a specific label. See the summary of this PR to when to use the memoized.inspect vs the WillBreakBuffer

@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 11:27 Inactive
///
/// let content = format_with(|f| {
/// let mut counter = Counter::default().memoized();
/// let counter_content = counter.inspect(f)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This API looks great! 🙀

@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 12:52 Inactive
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I've started to ponder if we should rename VecBuffer into ElementBuffer. Vec is way too generic, while it's actually the buffer where we want to write FormatElement

crates/rome_formatter/src/formatter.rs Show resolved Hide resolved
@MichaReiser
Copy link
Contributor Author

I've started to ponder if we should rename VecBuffer into ElementBuffer. Vec is way too generic, while it's actually the buffer where we want to write FormatElement

I'd like to keep VecBuffer because it best describes its data structure. The fact that we use into_element is more a hack and I ultimately want to get to a place where the Formatter uses an internal Interner that is shared between all memoized elements.

@ematipico
Copy link
Contributor

I'd like to keep VecBuffer because it best describes its data structure.

I guess we agree to disagree. I think that the data structure should be inferred from its types, and the name should explain its intent/meaning.

@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 14:13 Inactive
This PR adds the `inspect` method to `Memoized` that allows inspecting the formatted content.

This can be useful in scenarios where some formatting must determine if some content will break to decide the best formatting.

This is different from #2771 because the buffer should be preferred in cases where the fact whatever some content breaks is only used to determine the formatting of another element because it avoids interning/memoizing the inner content.

The API shown here should be preferred when the formatting of the element itself depends on whatever its content breaks where using the `WillBreakBuffer` would need to rewind to the previous state.
@MichaReiser MichaReiser temporarily deployed to aws June 24, 2022 14:21 Inactive
@MichaReiser MichaReiser merged commit eab8571 into main Jun 24, 2022
@MichaReiser MichaReiser deleted the feature/inspect-memoized branch June 24, 2022 14:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants