Add ExtSlice::escape_debug_into #37
Conversation
There was a problem hiding this comment.
Thanks so much for the PR! Overall this looks great and I appreciate the docs you added. However, I think there are a few issues I'd like to see addressed before merging though.
Also, for small PRs like this, could you make sure everything is squashed down to a single commit? Thanks. (Force pushing is okay.)
src/ext_slice.rs
Outdated
| /// assert_eq!(r"cannot load such file -- utf8-invalid-name-\xFF", message); | ||
| /// ``` | ||
| #[inline] | ||
| fn escape_debug_into<T: fmt::Write>(&self, f: &mut T) -> fmt::Result { |
There was a problem hiding this comment.
Why does this take an &mut T instead of a T? Also, could you use W as the type parameter?
There was a problem hiding this comment.
I thought I needed to do this to allow this function to be used in the fmt::Debug impl, but I didn't realize &mut &mut fmt::Formatter is also a fmt::Write.
src/ext_slice.rs
Outdated
| /// ``` | ||
| #[inline] | ||
| fn escape_debug_into<T: fmt::Write>(&self, f: &mut T) -> fmt::Result { | ||
| let buf = self.as_bstr(); |
There was a problem hiding this comment.
Why are you converting the slice to a &BStr first? It appears unnecessary and also results in an extra as_bytes() call below.
There was a problem hiding this comment.
this was a holdover from my own implementation of this function in my downstream project. I didn't realize all these functions are implemented on &[u8] too. fixed.
Add a method to the `ExtSlice` trait that supports writing the `BStr` escaped representation into a `fmt::Write`. This enables extracting the escaped `String` into a buffer without going through `fmt::Debug`. The written `String` does not contain the surrounding quotes present in the `fmt::Debug` implementation. Fixes BurntSushiGH-34. This change reimplements `fmt::Debug` for `BStr` with `ExtSlice::escape_debug_into`.
lopopolo
force-pushed
the
escape-debug-into
branch
from
dfa09ab
to
c266d44
Compare
|
Hi @BurntSushi thanks for the feedback. I think I've addressed everything. I did consider using |
- Rename `string::escape_unicode` to `string::format_unicode_debug_into`. - Rename writer type parameters to `W` for int and unicode debug formatters. - Remove `&mut` from writer parameter. - Remove the `impl From<fmt::Error> for WriteError` conversion. - Sync docs for `format_unicode_debug_into` from BurntSushi/bstr#37. While fixing up the use of `escape_unicode` in `artichoke-frontend`, I got a bit distracted and let the scope of this change creep. - Unify Ruby CLI errors on `Exception`. Imports `IOError` and `LoadError` to manually construct some errors from strings. - Do not use `eprintln!` in `artichoke` binary to suppress broken pipe errors instead of panicking. - Set exit code to 1 on error in `artichoke` binary.
- Rename `string::escape_unicode` to `string::format_unicode_debug_into`. - Rename writer type parameters to `W` for int and unicode debug formatters. - Remove `&mut` from writer parameter. - Remove the `impl From<fmt::Error> for WriteError` conversion. - Sync docs for `format_unicode_debug_into` from BurntSushi/bstr#37. While fixing up the use of `escape_unicode` in `artichoke-frontend`, I got a bit distracted and let the scope of this change creep. - Unify Ruby CLI errors on `Exception`. Imports `IOError` and `LoadError` to manually construct some errors from strings. - Do not use `eprintln!` in `artichoke` binary to suppress broken pipe errors instead of panicking. - Set exit code to 1 on error in `artichoke` binary.
There was a problem hiding this comment.
LGTM. Note:
I did consider using
[_]::getinstead of slice indexing to avoid a panic pathway in non performance critical code
Using get here wouldn't make sense, since the slice indexing can never fail. If it does fail, then it implies a bug in the char_indices implementation.
There was a problem hiding this comment.
All right, so I hate to do this, but I just spent some time reviewing how std does this, and this implementation path is inconsistent with std. I wish I realized this sooner instead of steering you down a wrong path.
Basically, I think the right way to do this is to mirror std: add an escape_debug method that returns an iterator of char values corresponding to the output. It's more complex to implement, but is a more flexible API and is consistent with std.
|
@BurntSushi can I defer this change to you? While it may be the right thing to do for bstr, it is unnecessary to meet my needs. |
|
Sure, but I have no idea when I'll get to it. |
Add a method to the
ExtSlicetrait that supports writing theBStrescaped representation into a
fmt::Write. This enables extracting theescaped
Stringinto a buffer without going throughfmt::Debug.The written
Stringdoes not contain the surrounding quotes present inthe
fmt::Debugimplementation.This PR implements
fmt::DebugonBStrwith the new method.Fixes GH-34.