-
-
Notifications
You must be signed in to change notification settings - Fork 398
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
Parser::SourceParser#convert_encoding - fixup BOM encoding #1510
Conversation
ae69ade
to
fe4f32e
Compare
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 think this would be good if it retained the initial equality check.
if content[0, bom.size] == bom | ||
content.force_encoding(encoding) | ||
return content | ||
if content.start_with?(bom) |
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.
Not sure there's a good enough reason to switch to this more restrictive (newer) API given the simplicity of the existing line.
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 changed it for two reasons, first, content[0, bom.size]
creates a string, second, both content
and bom
are both forced to binary
encoding, so there won't be any encoding issues with a character based test, which also happens with the existing check. Not sure why you're calling it 'more restrictive'?
Regardless, I can revert that change if you'd like...
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 remember start_with? being added in Ruby 2.5+, but it seems like it was added much earlier-- in which case this is fine.
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 remember start_with? being added in Ruby 2.5+
My memory isn't good for that sort of thing. CI did pass... I remember a starts_with?
a very long time ago. Thanks.
Thanks for the quick fix on #1509! |
Description
Ruby 3.3 may change handling of BOM encoded files. Change code to work with 3.3 and earlier versions.
Closes #1509
Note that Ruby 2.7 added
IO##set_encoding_by_bom
Completed Tasks
bundle exec rake
locally (if code is attached to PR).