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

Trailing newlines are calculated and represented in differ output. #290

Closed

Conversation

ChaeOkay
Copy link

@ChaeOkay ChaeOkay commented Jul 7, 2016

This is pretty much what @phiggins wrote in #71 :) but is up-to-date with master and rspec-expectations.

See commit message in 3a0af72 and rspec-expectations PR #290

@CoralineAda gave me the courage to dive in, and @alindeman let me borrow his eyes over coffee. I'm open to feedback, continued help, and working on next steps whatever they may be!

Fixes #70

*Backfills hunk_generator with extra newline to account for 'true'
trailing newlines that should be included in diff calculation.
  Example:

  "cat\n".split("\n") => []
  "cat\n\n".split("\n") => ["cat"]

  "cat\n".split("\n", -1) => ["cat", ""]
  "cat\n\n".split("\n", -1) => ["cat", "", ""]

*Removes extra newline representation from hunk_generator when crafting differ
output message.
@JonRowe
Copy link
Member

JonRowe commented Jul 7, 2016

Thanks for working on this!

@ChaeOkay
Copy link
Author

ChaeOkay commented Jul 7, 2016

Looks like we have a failing spec, looking into it!

@ChaeOkay
Copy link
Author

ChaeOkay commented Jul 7, 2016

Actually, @JonRowe any idea on what this could be https://travis-ci.org/rspec/rspec-support/jobs/142915946? I saw this encoding spec fail on master for me, and thought it might be something local.

def final_line(last_hunk)
output_message = last_hunk.diff(format_type).to_s.strip
# Checks last char of string for + or -, appends message on match
output_message.gsub(/(?<=[+-]$)\z/, "\\ No newline at end of input")
Copy link
Author

Choose a reason for hiding this comment

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

Escaping still needs work

Copy link
Member

Choose a reason for hiding this comment

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

Rather than doing this, have you tried leaving the old logic as it was, and just checking for the missing trailing \n?

Copy link
Author

Choose a reason for hiding this comment

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

I'm up for giving it another go! By 'old logic' do you mean

def expected_lines
  @expected.to_s.split("\n", -1).map! { |e| e.chomp }
end

in https://github.com/ChaeOkay/rspec-support/blob/b4a3bacd999429e7d64b88486e86597eefdbbb14/lib/rspec/support/hunk_generator.rb#L27 ?

Copy link
Member

Choose a reason for hiding this comment

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

The approach I think I'm suggesting, is to refactor expected_lines and actual_lines to check for a missing \n and add the text to the array if so, then allowing the differ to proceed as normal

Copy link
Member

@JonRowe JonRowe Jul 12, 2016

Choose a reason for hiding this comment

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

Something along the lines of:

def expected_lines
  split_into_lines(@expected)
end

def actual_lines
  split_into_lines(@actual)
end

private

def split_into_lines(string)
  string << "\ No newline at end of input" if string[-1] != "\n"
  string.split("\n").map! { |e| e.chomp }
end

This is psuedo code though, I haven't tried it directly

Copy link
Author

@ChaeOkay ChaeOkay Jul 12, 2016

Choose a reason for hiding this comment

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

Ah, okay.

Diff:
@@ -1,4 +1,5 @@
 Chewy
+
 bagel.
-\ No newline at end of input
+

Would this (above) be an acceptable output for

expected = "Chewy\nbagel."
actual = "Chewy\n\nbagel.\n"

The feature spec currently shows

Diff:
 @@ -1,3 +1,5 @@
  Chewy
  +
  bagel.
  +\ No newline at end of input

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to not have the trailing + unless it actually represents a proper \n\n, if you can't get quite that, feel free to push it up anyway I'll take another look :)

Copy link
Author

Choose a reason for hiding this comment

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

Eek, I've been away, but casually fiddling. Hope to move the discussion further, soon. Apologies!

Copy link
Author

Choose a reason for hiding this comment

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

  • If we are going to add text to indicate that there is an addition/deletion of an eof newline (instead of nothing at all), do we want this to happen before the actual or encoded string becomes and EncodedString?
  • How do we want to handle internationalization? Would the message always be in English?

@ChaeOkay
Copy link
Author

ChaeOkay commented Nov 9, 2016

It seems like we are uncovering additional specific requirements of this issue. I’m glad to have moved the conversation forward, but I’m not comfortable with the solution being put forth given the invasiveness required. Thanks @CoralineAda for your support. Thanks @JonRowe for taking me along this far, I’ve learned a ton about open source, diffing, and RSpec!

@ChaeOkay ChaeOkay closed this Nov 9, 2016
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