Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

HTML Reporter: Fixed the confusing Line Breaks in Diffs #764

Closed

Conversation

gauravmittal1995
Copy link
Contributor

Regarding #348
In the Below picture:
1st assert has same keys but different values,
2nd assert is for two strings,
3rd assert has different keys but similar values.

diff_pic

@gauravmittal1995
Copy link
Contributor Author

@leobalter @jzaefferer Please Review

@jzaefferer
Copy link
Member

Can you provide a screenshot of those same diffs without the changes of this PR applied?

@jzaefferer
Copy link
Member

Also #348 had an example where an object was replaced by an array. Could you cover that as well?

@gauravmittal1995
Copy link
Contributor Author

@jzaefferer for the object replaced by array:

list_array

@gauravmittal1995
Copy link
Contributor Author

Now for without the change (i.e originally):

pic1
pic2

@gauravmittal1995
Copy link
Contributor Author

@jzaefferer Hey, I also tried another way and got the following diff:

new_diff1
new_diff2

Please Let me know which one according to you is better.

@jzaefferer
Copy link
Member

Both approaches are definitely an improvement over the current implementation. The "another way" looks even better to me. We may need more tests to avoid regressions though.

@leobalter what do you think?

@gauravmittal1995
Copy link
Contributor Author

@jzaefferer the another way uses google's diff-patch-match library.

@jzaefferer
Copy link
Member

Ah, good to know. Could you create a separate PR for that?

@leobalter
Copy link
Member

I liked these changes, but I would like to see this other PR as it looks better.

@gauravmittal1995
Copy link
Contributor Author

@jzaefferer @leobalter Creating the new PR. Its taking time because have to refactor as it doesnt use cameCase and vars are scattered throughout the function. Will send it soon

@leobalter
Copy link
Member

As it is an external library, it would be good to ignore it on jshint and jscs checks. Also, getting the code from a npm dependency would be nice.

@gauravmittal1995
Copy link
Contributor Author

@leobalter @jzaefferer PR opened #772

@gauravmittal1995
Copy link
Contributor Author

@jzaefferer I think its safe to close this PR as we r mostly gonna stick with #772 ??

@leobalter
Copy link
Member

Yes, @gauravmittal1995, thanks!

@leobalter leobalter closed this Mar 17, 2015
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants