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

Created ListDiff class to handle diffing of lists. #14

Merged
merged 12 commits into from
Oct 20, 2015

Conversation

adamCaxy
Copy link
Contributor

Created ListDiff class to handle diffing of lists.

Created ListDiff and ListNode classes.

Started on custom list diffing functionality.
Finished list diffing against nodes and returning the result.
@jschroed91 jschroed91 self-assigned this Sep 28, 2015

protected function diffList($oldText, $newText)
{
$diff = new ListDiff($oldText, $newText, $this->encoding, $this->isolatedDiffTags, $this->groupDiffs);
Copy link
Member

Choose a reason for hiding this comment

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

The 4th argument here should actually be $this->specialCaseTags, not $this->isolatedDiffTags -- the same goes for the call to HtmlDiff on line 206, I missed that in the review :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@jschroed91
Copy link
Member

I've been encountering some errors testing with the demo, some of them would be fixed by the comments suggested but I think there may be some additional issues with how the nested placeholders are processed.

Here is a Gist with "old text" and "new text" that I've been testing with, taken from a real example in cdpACCESS: https://gist.github.com/jschroed91/ed6a21856b86e5ea7ff1

While testing on the demo, I suggest updating line 6 in the demo/index.php file to be error_reporting(E_ALL), and then take a look at the errors / notices that are output -- it's quite helpful.

I made some updates locally to fix the issue when lists have attributes (like class), and landed on an issue that appears to be related to processing the nested placeholders:
selection_320

Next issue to be resolved: <li> content is not separated... all on one line.

Fixes were in for this, but I screwed it up and had to revert backwards like 4 hours.
… the rest of the diffing functionality!

Now need to finish the update so that these changes work with the rest of the script.
@jschroed91
Copy link
Member

This is looking good to me, I'm going to run some tests on it locally for some specific use cases to see if we're covering some of the complex lists.

Also, looks like Scrutinizer was acting up so I scheduled a new inspection on the branch and will be waiting on that.

jschroed91 added a commit that referenced this pull request Oct 20, 2015
Created ListDiff class to handle diffing of lists.
@jschroed91 jschroed91 merged commit 5d492a4 into master Oct 20, 2015
@jschroed91 jschroed91 deleted the feature-list_diffing-new branch October 20, 2015 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants