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

Don't disregard trailing newlines in Differ input. #70 #71

Conversation

phiggins
Copy link
Contributor

A fix for #70.

String#split takes an optional second argument. From the docs:

If the limit parameter is omitted, trailing null fields are suppressed. If limit is a positive number, at most that number of fields will be returned (if limit is 1, the entire string is returned as the only entry in an array). If negative, there is no limit to the number of fields returned, and trailing null fields are not suppressed.

The difference being output like this:

 >   "foo\nbar\n".split("\n")
 => ["foo", "bar"] 
 > "foo\nbar\n".split("\n", -1)
 => ["foo", "bar", ""] 
 > "foo\nbar\n\n\n".split("\n", -1)
 => ["foo", "bar", "", "", ""]

I modified the methods that preprocessed the input to Differ to keep these empty strings for any trailing newlines. This worked to fix the issue in #70, but I had to modify lots of Differ's test cases since they used a trailing newline to trigger their diffability.

diff = differ.diff "abc", "def"

expect(diff).to be_empty
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.

💭 This wasn't really related to this issue, but it was a missing test case so I threw it in just for completeness's sake.

@JonRowe
Copy link
Member

JonRowe commented May 27, 2014

Currently the build is failing with incorrect or unexpected diffs...

@phiggins
Copy link
Contributor Author

@JonRowe Thanks for the heads up, I opened a PR against rspec-expectations to fix the specs that depended on Differ.

@phiggins
Copy link
Contributor Author

The branch I opened on rspec-expectations, rspec/rspec-expectations#556, is passing, but this build is still failing. Is there some way I can make this use my branch of rspec-expectations on CI?

@JonRowe
Copy link
Member

JonRowe commented May 28, 2014

Generally our procedure is to add a temporary commit linking the Gemfile to the branch, and then when passing, delete the commit and merge things simultaneously.

@phiggins
Copy link
Contributor Author

Generally our procedure is to add a temporary commit linking the Gemfile to the branch, and then when passing, delete the commit and merge things simultaneously.

I did that, unless I'm misunderstanding what you mean.

@JonRowe
Copy link
Member

JonRowe commented May 28, 2014

You're not, for whatever reason your build is still failing against your branch

@phiggins phiggins closed this Jun 10, 2014
@myronmarston
Copy link
Member

Is this not an issue anymore, @phiggins? Sorry I never got around to reviewing it...

@phiggins
Copy link
Contributor Author

It seemed like I was having to make pretty invasive changes to fix a minor edge case bug. The issues are still open so if there's interest these can be reopened.

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.

3 participants