CharProperty::display() ?
behnam opened this issue · 11 comments
Do we want to add display() as an instance method to CharProperty API? We already have impl fmt::Display for it, but returning &str would be useful in cases that the display string is not expected to be matched with formatting.
I have seen both cases in third-party libraries.
What do you think?
display() would have to return String for the case when we turn Long_Name into Long Name via replace('_', " ").
Because of that, I'm slightly in favor of just letting consumers use format!("{}", property).
display()would have to returnStringfor the case when we turnLong_NameintoLong Nameviareplace('_', " ").
Either way, we will use the replace('_', ' ') call as a fallback. So, I'm not sure what you're implying here. Do you mean that display() is no necessarily cheap, as someone would expect?
Here I'm specifically mentioning that fn display(&self) -> &str is impossible. Because we are unable to provide a version returning &str, I think it is best to just use the format! call to return a String.
If we skip the replace() call, which is just a bit of fanciness, we can keep the return type as &str.
If we skip the modification of the long name, it would be worth it to provide the display fn returning &str.
Idea:
fn abbr_name(&self) -> Option<&'static str>;
fn long_name(&self) -> Option<&'static str>;
fn rust_name(&self) -> &'static str;
fn display_name(&self) -> &'static str {
self.long_name()
.or(self.abbr_name)
.unwrap_or(self.rust_name())
}
fn Display::fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "{}", self.display_name())
}
EDIT: Scratch that, doesn't work as is
Yeah, makes sense to have something like display_name(), and on which display() would fallback when no display string is defined.
The only thing is naming that function, since display_name() can be confused with display() itself, which is kind of returning a name already, but tries to be more human-friendly.
We can call it best_name() or just name(). That's the best I can come up with right now... :/
The idea (really roughly presented) was actually that display_name would contain the functionality currently in the impl of Display::fmt, which would then just call display_name. Unfortunately Display still has to be impl'd on a by-struct basis since you cannot do it over a type parameter.
Okay, display() vs fmt::Display::fmt() is one issue. We don't have to decide on that right now.
Now, we have a bunch of optional strings defined for each variant in the marco. We need a consistent API for accessing them. I think for every attribute xyz, the accessor method for the string should be either xyz() or xyz_name(). We're already going with the latter, which is fine. We will need to add display_name() as an accessor for display string, though.
In addition to those, we also have rust_name(), which is okay, but I don't really like to see rust_ prefix in my rust code. I recommend to either use simple name(), or even more crazy like to_str(), or both!
Then, we have this fancy try-to-be-ver-user-friendly composed logic, which starts from display, and falls back to long, abbr and variant/rust name. I think it's okay to have this specific function return String and keep the long_name().replace('_', ' ') logic in it. This could be directly under fmt::Display::fmt(), or we can put it in a method and have it called from Display.
And, now writing this, I realize that we may want to rename that display attribute name to something better, to not overlap concepts with Display. How about human or something like that? (nice, fancy, or even desc, ...)
human feels right to me. desc is nice as it's the same length, but that's not really a metric we should chose naming off of. "Human name" better describes what you should be providing in any case. So we'd have:
LeftToRight {
abbr => L,
long => Left_To_Right,
human => "Left-to-Right",
}
LeftToRight.abbr_name() => Some("L")
LeftToRight.long_name() => Some("Left_To_Right")
LeftToRight.human_name() => Some("Left-to-Right")
// The variant name really is not necessary to provide.
// Abbr/Long make sense as UCD-defined names
// But the rust name is just an adaptation to Rust naming conventions
(This sketch returns Option to allow leaving out of abbr/long/human for those properties that don't have them.)
By leaving out the "rust name" fallback in Display::fmt (or name() or whatever other method), we could even have Rust match-exhaustiveness check enforce that at least one is present, if we want to.
I still stand by my point before that if we just return a String anyway, just use Display and people can call format!.
I still stand by my point before that if we just return a String anyway, just use Display and people can call format!.
Yeah, that's a good call. For the logic that would return String, let's have it be called via Display/format.