-
Notifications
You must be signed in to change notification settings - Fork 18
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
Conversation
@@ -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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clean! lgtm
Co-authored-by: Joe Caputo <joseph.caputo@unfinished.com>
Co-authored-by: Matthew Orris <1466844+mattheworris@users.noreply.github.com>
There was a problem hiding this 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)
There was a problem hiding this 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!
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:
Checklist