Remove feature gating from detect_encoding()#952
Conversation
dralley
force-pushed
the
detect-encoding
branch
from
7c3d783 to
6c8e6b2
Compare
| use crate::reader::EncodingRef; | ||
| #[cfg(feature = "encoding")] | ||
| use encoding_rs::{Encoding, UTF_8}; | ||
| use encoding_rs; |
There was a problem hiding this comment.
I wanted it to be 100% clear where types were coming from, as some of the constants in mod encoding are very similar to other types or constants from encoding_rs, and especially with arguments and return values it's best that it be clear they are foreign types.
dralley
force-pushed
the
detect-encoding
branch
from
6c8e6b2 to
4ed2d30
Compare
|
|
||
| use tokio::io::{self, AsyncBufRead, AsyncBufReadExt, AsyncRead, ReadBuf}; | ||
|
|
||
| use crate::encoding; |
There was a problem hiding this comment.
Used by the macros.
| if let Some(encoding) = $reader.detect_encoding() $(.$await)? ? { | ||
| if $self.state.encoding.can_be_refined() { | ||
| $self.state.encoding = crate::reader::EncodingRef::BomDetected(encoding); | ||
| $self.state.encoding = crate::reader::EncodingRef::BomDetected(encoding.encoding()); |
There was a problem hiding this comment.
As mentioned on the previous PR, detect_encoding() returns / returned Some() even in cases where no BOM was found, thus this is perhaps misnamed.
However, the initial XML characters function like a BOM, and the documentation mentions it, so I see an argument for leaving this be.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #952 +/- ##
==========================================
+ Coverage 55.08% 56.40% +1.31%
==========================================
Files 44 44
Lines 16911 17607 +696
==========================================
+ Hits 9316 9931 +615
- Misses 7595 7676 +81
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dralley
force-pushed
the
detect-encoding
branch
from
4ed2d30 to
c0cab37
Compare
|
After this PR, can we go ahead and cut a release that includes the attribute value normalization? |
Agree, I also want to release new version soon. I will review this and try to fix the latest issues and then release new version. If the latest issues will hard to fix quickly, then I'll release new version without their fixes. |
Mingun
left a comment
Mingun
left a comment
There was a problem hiding this comment.
It seems two small improvements could be made, but other is good
| use crate::reader::EncodingRef; | ||
| #[cfg(feature = "encoding")] | ||
| use encoding_rs::{Encoding, UTF_8}; | ||
| use encoding_rs; |
Have it return a generic enum so that it can be used with or without the encoding feature.
|
re-pushed |
3 participants
Have it return a generic enum so that it can be used with or without the encoding feature.
It was suggested that this be split off from #947