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

Display handles are whitespace trimed and collapsed #2157

Merged
merged 5 commits into from
Sep 17, 2024

Conversation

wilwade
Copy link
Collaborator

@wilwade wilwade commented Sep 17, 2024

Goal

The goal of this PR is to ensure that all new base handles (and thus the resulting display handle) have whitespace trimmed and concatenated

Closes #2148

Discussion

Fairly straightforward. I did benchmark against the collapse crate, and tested some with the versions here: https://stackoverflow.com/questions/71864137/whats-the-ideal-way-to-trim-extra-spaces-from-a-string

In the end this one benchmarked the fastest using a version of this:

#[cfg(test)]
mod tests {
    use super::*;
    use rand::distributions::Alphanumeric;
    use rand::distributions::DistString;
    use test::Bencher;

    #[bench]
    fn bench_one(b: &mut Bencher) {
        let mut rng = rand::thread_rng();
        b.iter(|| {
            trim_and_collapse_whitespace(
                Alphanumeric
                    .sample_string(&mut rng, 20)
                    .to_lowercase()
                    .replace("a", " ")
                    .replace("b", " ")
                    .as_str(),
            );
        });
    }
}

Checklist

  • Updated Pallet Readme?
  • Design doc(s) updated?
  • Unit Tests added?
  • Spec version incremented?

@wilwade wilwade requested review from a team, shannonwells, mattheworris, enddynayn, aramikm, claireolmstead and JoeCap08055 and removed request for a team September 17, 2024 14:23
@@ -711,8 +713,9 @@ pub mod pallet {
base_handle: &str,
suffix: HandleSuffix,
) -> Result<DisplayHandle, DispatchError> {
let base_handle_trimmed = trim_and_collapse_whitespace(base_handle);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Believe it or not, this appears to be the place that builds the display handle that is saved.

@@ -84,7 +84,7 @@ pub fn split_display_name(display_name_str: &str) -> Option<(String, HandleSuffi
let parts: Vec<&str> = display_name_str.split(HANDLE_DELIMITER).collect();
let base_handle_str = parts[0].to_string();
if parts.len() != 2 {
return None
return None;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My formatter really wants to have ; at the end of the line when it is return

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
pallets/handles/src/handles-utils/src/converter.rs 100.00% <100.00%> (ø)
pallets/handles/src/lib.rs 91.48% <100.00%> (+0.03%) ⬆️

Copy link
Collaborator

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

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

clean! lgtm

@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Sep 17, 2024
Co-authored-by: Joe Caputo <joseph.caputo@unfinished.com>
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label Sep 17, 2024
Co-authored-by: Matthew Orris <1466844+mattheworris@users.noreply.github.com>
Copy link
Collaborator

@JoeCap08055 JoeCap08055 left a comment

Choose a reason for hiding this comment

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

Looks good--might want to take a pass through the comments/docs and make sure we are consistent in how we refer to this operation: either "collapse" or "consolidate" white space, but NOT "concatenate".

One toolkit I worked with a while back referred to this operation as "simplifying" white space (ie, the combination of trim + collapse)

@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label Sep 17, 2024
Copy link
Collaborator

@mattheworris mattheworris left a comment

Choose a reason for hiding this comment

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

  • Read through changes
  • Executed tests

🚢 it!

@wilwade wilwade enabled auto-merge (squash) September 17, 2024 16:12
@wilwade wilwade merged commit 7d1f964 into main Sep 17, 2024
27 checks passed
@wilwade wilwade deleted the feature/trim-display-handles branch September 17, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metadata-changed Metadata has changed since the latest full release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trim Display Handle Whitespace
4 participants