-
-
Notifications
You must be signed in to change notification settings - Fork 390
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
adds CESU-8 encoding for encoding spec #805
Conversation
core/string/valid_encoding_spec.rb
Outdated
@@ -98,6 +98,9 @@ | |||
str.force_encoding('MacJapanese').valid_encoding?.should be_false | |||
str.force_encoding('UTF-7').valid_encoding?.should be_true | |||
str.force_encoding('UTF8-MAC').valid_encoding?.should be_true | |||
ruby_version_is "2.7" do | |||
str.force_encoding('CESU-8').valid_encoding?.should be_true |
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.
Could you move this in a separate it
?
The reason is otherwise the entire spec example would fail on some Ruby implementation (reporting >= 2.7) which didn't add CESU-8 yet.
In general, ruby_version_is
should be outside it
blocks.
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.
Maybe a better place to test this would be the Encoding.list spec or so.
Testing specific characteristics of CESU-8 (e.g., how some codepoint is encoded) would be even better.
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.
@eregon Thanks for review!
Could you move this in a separate it?
Sure!
Maybe a better place to test this would be the Encoding.list spec or so
As I see now there no tests for specific encodings in core/encoding/list_spec.rb
. Maybe we should add something like that?
it 'includes CESU-8' do
Encoding.list.include?(Encoding::CESU_8).should be_true
end
it 'includes UTF-8' do
Encoding.list.include?(Encoding::UTF_8).should be_true
end
...
# and so on
Testing specific characteristics of CESU-8 (e.g., how some codepoint is encoded) would be even better.
Something like in Rust docs?
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.
Maybe we should add something like that?
Feel free to add UTF-8 too, but I wouldn't add all the others.
CESU-8 is special in that it's a newly-added Encoding.
Yes, for instance testing 10400.chr(Encoding::CESU_8).bytes.should == [...]
(from the Wikipedia example).
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.
@eregon I have added more specific specs. Can you check please?
cd9141c
to
c71bfa5
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.
Looks great, thanks for the new specs!
Solving #745