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.

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

Test Differ#pick_encoding; move to EncodedString #167

Merged
merged 2 commits into from
Feb 8, 2015

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Feb 3, 2015

Followup to #151

especially see #134 (comment)

@@ -7,6 +7,7 @@ module RSpec
module Support
# rubocop:disable ClassLength
class Differ
LINE_BREAK = "\n"
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning behind this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, as I commented in bf4@ce333af I was thinking of using LINE_BREAK as a way to get the __ENCODING__ of strings generated in differ.rb, that also happened to be a nice cleanup of string generation. Turns out it didn't work like I thought it did, so, the only reason to keep it now is cleanup, if you like. I'm going to try again with a less aggressive strategy.

@bf4
Copy link
Contributor Author

bf4 commented Feb 6, 2015

Well, the tests all pass now.

I'm thinking of simplifying this PR to

  1. Testing Differ#pick_encoding
  2. Moving pick_encoding into EncodedString

Then, in another PR, per any discussion in this one, change the tests in the differ_spec.rb to use an encoding-agnostic be_identical_string matcher, such as in this changset, or just use object equality, for now.

Then, with better test coverage, I'll return to the original exception that started all this, an ArgumentError on (Encoded)String#split. Maybe we should just bundle string-scrub (extracted from Ruby 2.1, 2)?

@bf4
Copy link
Contributor Author

bf4 commented Feb 6, 2015

Since I'm going to revise this PR a bit, I thought I might as well push the ArgumentError on EncodingString#split fix for a preview of what it is, though it would go into a subsequent PR.

# HACK: To disable the encoding check
chain :without_encoding do
@without_encoding = :without_encoding
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: the hack here and below, I'm wondering if I should change the name of the matcher to expect_identical_encoded_string and expect_identical_string and see if there are still any cases for chaining

@bf4 bf4 changed the title Test Differ#pick_encoding; don't use differ on differ failures Test Differ#pick_encoding; move to EncodedString Feb 8, 2015
@bf4
Copy link
Contributor Author

bf4 commented Feb 8, 2015

I want to keep this moving, so I've reduced this PR to just the Differ/EncodedString#pick_encoding

JonRowe added a commit that referenced this pull request Feb 8, 2015
Test Differ#pick_encoding; move to EncodedString
@JonRowe JonRowe merged commit cb7d560 into rspec:master Feb 8, 2015
@JonRowe
Copy link
Member

JonRowe commented Feb 8, 2015

Thanks @bf4

@JonRowe
Copy link
Member

JonRowe commented Feb 8, 2015

(Note @myronmarston I haven't added a change log because this is pretty much just an internal refactoring)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants