-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add bom_sniffing option to DecodeReaderBytesBuilder #5
Conversation
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.
This looks great! Thanks so much. This was easier to implement than I thought. :-)
I left a couple small nits, but after that, this should be good to go.
src/lib.rs
Outdated
let has_detected = !self.bom_override && encoding.is_some(); | ||
|
||
// No need to do BOM detection if we opt out of it or have an explicit encoding. | ||
let has_detected = !self.bom_sniffing || !self.bom_override && encoding.is_some(); |
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.
Can you add parens here to indicate precedent?
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.
Done!
src/lib.rs
Outdated
/// the encoding with BOM. | ||
/// | ||
/// When this is disabled and an explicit encoding is not set, the decoder will treat the input | ||
/// as raw bytes. |
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.
I would add, "The bytes will be passed through unchanged, including any BOM that may be present."
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.
Done!
src/lib.rs
Outdated
let has_detected = !self.bom_override && encoding.is_some(); | ||
|
||
// No need to do BOM detection if we opt out of it or have an explicit encoding. | ||
let has_detected = !self.bom_sniffing || (!self.bom_override && encoding.is_some()); |
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.
Sorry, I just noticed this: can you make sure all new code introduced in this is wrapped to 79 columns inclusive? It should follow the existing style of the code. This line in particular is longer than 79. The comments introduced below are also longer than 79 columns.
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.
Done. I wanted to use cargo fmt
at first but didn't know if this repository follows it's rules. Perhaps this tool can be used in the future?
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.
It'd be nice, but no, I don't think rustfmt is in a place yet for me to adopt it. It does too many sub-optimal things with the formatting.
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.
Awesome!
This PR is now in |
@BurntSushi Great, thanks. I will work on the "plumbing" part now. :) |
BurntSushi/ripgrep#1207