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

Updates from changes in rspec-support #929

Closed

Conversation

ChaeOkay
Copy link

@ChaeOkay ChaeOkay commented Jul 7, 2016

@@ -1,3 +1,4 @@
Chewy
bagel.
+
Copy link
Member

Choose a reason for hiding this comment

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

This output is slightly wrong, it's actually -bagel. +bagel. but with a correct line ending, I'm not sure how to reflect this yet but this isn't the way. (Posix specification specifies all lines end with a \n, git would normally warn about the line ending at the end)

Copy link
Author

Choose a reason for hiding this comment

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

Ah right.

Copy link
Member

Choose a reason for hiding this comment

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

Git prints \ No newline at end of file perhaps we should be looking to print \ 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.

Actually, this output is what I'm suggesting as the fix :)

From what I understood, the goal was to represent a trailing newline in the diff output instead of nothing at all. This option represents trailing newline diffs the same ways that other newlines diffs in non-trailing situations do.

I explored adding a string representation where trailing newlines occurred as git does, but it became pretty involved since we add and remove trailing newlines as the output message is crafted in several areas of the code.

Do you have any ideas of what you'd like to see instead?

Copy link
Author

@ChaeOkay ChaeOkay Jul 7, 2016

Choose a reason for hiding this comment

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

Just for the records, google-diff-match-patch represents trailing newlines no different than other newlines.

Are you sure you want all of the code involved with putting in \ No newline at end of input? I'm totally for it, just checking one more time :)

Copy link
Author

Choose a reason for hiding this comment

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

Seems reasonable, will continue at it!

Copy link
Author

Choose a reason for hiding this comment

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

Hm, do we want a + or - in front of \ No newline at end of input depending on how it relates to the expected?

Also why I thought maybe just a + or - might be acceptable:
screen shot 2016-07-06 at 9 16 30 pm
screen shot 2016-07-06 at 9 15 53 pm

Copy link
Member

Choose a reason for hiding this comment

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

If you looked at the characters of that file you would find Chewy\nbagel.\n\n Chewy\nbagel.\n

Copy link
Member

Choose a reason for hiding this comment

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

Having +\ No newline at end of input -\ No newline at end of input works for me.

Copy link
Author

Choose a reason for hiding this comment

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

Updated specs, still working on escaping, not sure what's going on there.

So, I was using VIM to create t1.txt! I ended up re-learning $ echo -n to see the ouput message we are talking about! Thanks for being patient with me.

-/expected/m
+this is the
+ actual
+ string
"""

Scenario: diff for a multiline string and a regexp
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a duplicate of another scenario, was this committed accidentally?

Copy link
Author

Choose a reason for hiding this comment

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

Yes.

@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