Skip to content
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.

Sign up for GitHub

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

Jump to bottom

adds IO#set_encoding_by_bom spec #813

Merged
merged 2 commits into from
Nov 3, 2020
Merged

adds IO#set_encoding_by_bom spec #813

merged 2 commits into from
Nov 3, 2020

Conversation

moofkit
Copy link
Contributor

@moofkit moofkit commented Nov 2, 2020

Solving #745

Added IO#set_encoding_by_bom to check the BOM and set the external
encoding. Bug #15210

eregon
eregon reviewed Nov 2, 2020
View reviewed changes
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the new specs!

core/io/set_encoding_by_bom_spec.rb
not_binary_io = new_io(@name, 'r')

-> { not_binary_io.set_encoding_by_bom }.should raise_error(ArgumentError, 'ASCII incompatible encoding needs binmode')
not_binary_io.close
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this in a begin/ensure/end? So even if the assertion fails, the IO is still closed.

core/io/set_encoding_by_bom_spec.rb

@io.set_encoding_by_bom.should == Encoding::UTF_8
@io.external_encoding.should == Encoding::UTF_8
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add examples for UTF-16LE and UTF16-BE BOM?

core/io/set_encoding_by_bom_spec.rb Outdated

ruby_version_is "2.7" do
it "returns the result encoding if found BOM UTF-8 sequence" do
File.write(@name, "\u{FEFF}abc")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File.binwrite seems better here

eregon
eregon approved these changes Nov 3, 2020
View reviewed changes
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, looks great.

@eregon eregon merged commit f975ca6 into ruby:master Nov 3, 2020
@moofkit
Copy link
Contributor Author

moofkit commented Nov 3, 2020

@eregon thanks for review!

@moofkit moofkit deleted the io_set_encoding_by_bom branch November 3, 2020 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eregon eregon eregon approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@moofkit @eregon