Re-add Display trait on descriptors#54
Conversation
| impl<'a> fmt::Display for ClassName<'a> { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { | ||
| let segments : Vec<Cow<'a, str>> = self.segments.iter().map(|s| s.name.clone()).collect(); | ||
| write!(f, "{}", segments.join("/")) |
There was a problem hiding this comment.
Joining on / isn't quite perfect here as I think the segments can also separated by ;. Thoughts?
There was a problem hiding this comment.
There's just one ; at the end, to mark the end of the class name descriptor. So this is fine, just needs a ; appended
There was a problem hiding this comment.
Hmm. Unclear why but the addition of ; breaks things for me in the jaffi PR.
There was a problem hiding this comment.
Oh I missed the ; was being added at the callsite where you serialize the L{}; - so your original implementation was right, don't need the extra ;, sorry!
There was a problem hiding this comment.
Thanks, I've merged that and some test changes too. Published everything as 0.8.1. Cheers!
staktrace
left a comment
staktrace
left a comment
There was a problem hiding this comment.
This looks good, just a couple of comments below.
| impl<'a> fmt::Display for ClassName<'a> { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { | ||
| let segments : Vec<Cow<'a, str>> = self.segments.iter().map(|s| s.name.clone()).collect(); | ||
| write!(f, "{}", segments.join("/")) |
There was a problem hiding this comment.
There's just one ; at the end, to mark the end of the class name descriptor. So this is fine, just needs a ; appended
|
|
||
| impl fmt::Display for FieldDescriptor<'_> { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { | ||
| if self.dimensions > 0 { |
There was a problem hiding this comment.
Don't need the if/else here, if dimensions is 0 then the repeat call will already return an empty string. So the if branch already handles both cases correctly
|
Thanks for the quick review and merge! If I can't fix the jaffi side of things nicely, I might author more changes here! |
2 participants
Hey hey,
I appreciate this crate and the work you do!
I was going about upgrading from 0.6 to 0.8 in
jaffiand noticed thatimpl Displaywas removed in #47.So I re-added it as it's used for the method name generation. This is an example of the name generation. If I use the
Debugrepresentation it's pretty gross (and would also be a breaking change).