Extracting EncodedString specs from #134 #151
Conversation
bf4
force-pushed
the
encoding_specs
branch
2 times, most recently
from
e6e04bc
to
601924a
Compare
| # | ||
| # Raised by byte <-> char conversions | ||
| # RangeError: out of char range | ||
| # e.g. the UTF-16LE emoji: 128169.chr |
There was a problem hiding this comment.
Is this too much detail in the comments?
There was a problem hiding this comment.
The detail is helpful, actually.
There was a problem hiding this comment.
Nice docs. There's not enough about ruby encodings around the place so this is great.
| # and "\x80".encode('UTF-8','ASCII-8BIT', undef: :replace, replace: '<undef>') | ||
| # # => '<undef>' | ||
| # Encoding::CompatibilityError | ||
| # when Enconding.compatbile?(str1, str2) is false |
There was a problem hiding this comment.
this is technically incorrect. as written, it's when nil. I'll revise if everything else is ok.
I was confusing enc.ascii_compatible?
* Returns whether ASCII-compatible or not.
*
* Encoding::UTF_8.ascii_compatible? #=> true
* Encoding::UTF_16BE.ascii_compatible? #=> falsewith rb_enc_check
rb_enc_check(VALUE str1, VALUE str2)
{
rb_encoding *enc = rb_enc_compatible(str1, str2);
if (!enc)
rb_raise(rb_eEncCompatError, "incompatible character encodings: %s and %s",There was a problem hiding this comment.
Yeah, please fix this. (Good catch!). Also, s/compatbile/compatible/
|
I opened a PR against your branch that refactors the matcher and does a couple other small changes: |
oh fun. Will review |
|
Didn't finish addressing comments tonight. Merged and amended last commit in your PR. Will finish and push code 'tomorrow' 💤 |
|
Didn't work on it today. Probably won't again till Sunday. |
|
That's fine :). |
- Add tests for EncodedString#to_s, #split, #<< - For each Encoding failure - assert the expected failure is raised o a String, but not on an 'EncodedString' - assert invalid bytes or unconvertale characters are replaced Currently one test is failing (pending) - EncodedString#split when the string has an invalid byte sequence incorrectly raises an ArgumentError Use 'expect_identical_string' to avoid running expectation failures through the differ, which also uses EncodingString
It’s actually not quite the hot spot it says it here. Encoded strings are only created when an expectation fails that uses a diffable matcher. I believe that expectations normally pass (since people usually try to keep their test suite green and only have a small number of failing specs at a time), so encoded strings are not created by every expectation.
These cases are contrasts (as one example raises the error, but the other does not), so using `and` was confusing since they don’t do the same thing. `vs` makes more sense.
This reads better, provides better failure output, and is composable.
|
Rebased off of current master and force pushed Add new commits to PR per discussion for easy review, if ok, would like to discuss which concepts in the pr deserve their own commit and how your PR into my PR should appear. (if you don't care, I'm comfortable just doing something). Also, I looked into windows encodings more and think I'd like to document how to set the default external encoding that the suite expects on posix and windows, see #151 (comment) and #151 (comment). |
|
@myronmarston This is ready for review, when you get the chance. (Also see previous comment). |
Extracting EncodedString specs from #134
|
Merged. Thanks, @bf4! |
|
🌈 🎆 🐴 💯 yay! |
| # vs "\x80".encode('UTF-8','ASCII-8BIT', undef: :replace, replace: '<undef>') | ||
| # # => '<undef>' | ||
| # Encoding::CompatibilityError | ||
| # when Enconding.compatbile?(str1, str2) is false |
See #134
I need help with the matcher
Also, I decided not to include the new differ specs in here,
to reduce complexity, but am open to adding it.