Skip to content

Fixing Cluster Info + adding cluster layout snapshots #402

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

taj-p
Copy link
Contributor

@taj-p taj-p commented Aug 24, 2025

Intent

I noticed that, although my manual tests were passing, the source_char used for a given cluster wasn't always correct. I think we were lucky (if that's the right word) that characters that contribute to a ligature typically mimic the properties of other characters in that ligature.

This PR corrects those mistakes and adds more tests and documentation.

check_cluster_snapshot

I think it's pretty tricky to ensure that returned clusters are correct so I've added a new snapshot type enabled via check_cluster_snapshot which tries to visualise cluster information (and ensure correct text_len). This should make push_run more resilient to regressions and maintenance easier.

image


/// Returns the text length of the cluster in bytes.
#[cfg(test)]
pub(crate) fn text_len(&self) -> u8 {
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'd really like to test this in check_cluster_snapshot, so I've exposed it for tests only.

@@ -100,6 +99,11 @@ impl ClusterInfo {
pub(crate) fn is_whitespace(&self) -> bool {
self.source_char.is_whitespace()
}

#[cfg(test)]
pub(crate) fn source_char(&self) -> char {
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'd really like to test this in check_cluster_snapshot, so I've exposed it for tests only.

@taj-p taj-p marked this pull request as ready for review August 24, 2025 20:05
@taj-p taj-p requested a review from dfrg August 24, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant