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

php-diff-82 #86

Merged
merged 29 commits into from
Oct 26, 2021
Merged

php-diff-82 #86

merged 29 commits into from
Oct 26, 2021

Conversation

DigiLive
Copy link
Collaborator

Proof of concept - Ignore blank lines.

Adds option IgnoreLines which accepts 3 levels:
0 (or any other value): Don't ignore blank lines.
1: Ignore empty lines.
2: Ignore blank lines. (Contains no or only whitespace characters).

Please note the script spawns errors.
At this moment, ignoring lines is only implemented for the HTML SideBySide renderer.

Once this feature operates as expected, it can be extended to the other renderers.

DigiLive added 3 commits December 10, 2020 08:00
* Add missing documentation.
* Fixes and reformatting.
* Reduction of cyclomatic complexity.
* Add option `ignoreLines` to control the ability to ignore blank or
  empty lines.
* Add styles of ignored lines to example css.
* Add generator for ignored lines to HTML SideBySide renderer.
* Add `options` property to the Sequence Mather.
* Refactor option `ignoreNewLines` of the Sequence Matcher.
* Add flags to define the level of ignoring blank lines.
* Add tag `ignored` to the opCodes.
* Add method stripLines to Similarity.
* Add default option `ignoreLines` to Diff.
* Add tests for testing option `ignoreLines` of the Sequence Matcher.
* Reformat code.
@DigiLive DigiLive requested a review from JBlond December 10, 2020 08:03
@DigiLive DigiLive marked this pull request as draft December 10, 2020 08:03
@DigiLive DigiLive linked an issue Dec 10, 2020 that may be closed by this pull request
@DigiLive
Copy link
Collaborator Author

Do we trigger a deprecation notice when a sub renderer doesn't have the new generateLinesIgnore() method?

This while updating the included sub renderers of course. It's meant for users custom renderers.

@JBlond
Copy link
Owner

JBlond commented Dec 11, 2020

We could check if the interface is implemented or use https://github.com/Roave/you-are-using-it-wrong or use https://github.com/Roave/Dont

@DigiLive
Copy link
Collaborator Author

We could check if the interface is implemented or use https://github.com/Roave/you-are-using-it-wrong or use https://github.com/Roave/Dont

I'm not familiar with those, but I was thinking something simple like:

                if (!method_exists($subRenderer, 'generateLinesIgnore')) {
                    trigger_error(
                        'The use of a subRenderer without method generateLinesIgnore() is deprecated!',
                        E_USER_DEPRECATED

@JBlond
Copy link
Owner

JBlond commented Dec 16, 2020

Either another abstract class that implements SubRendererInterface

AND OR

public function __construct()
{
	if(!class_implements('SubRendererInterface')){
		// throw new error; // set your error here..
	}
}

JBlond and others added 10 commits December 18, 2020 15:10
Instead of defining constants with the same name and values at different
 namespaces, they're now defined at an interface which can be shared
among classes by implementing this interface.
If a used sub renderer, which extends the main renderer, a deprecation
notice will be thrown when the sub renderer is missing method
`generateLinesIgnore`.
Currently the line is commented out, because the method is not required
 until the deprecation period for missing this method is over.
For calculating the similarity ratio when empty/blank lines are
ignored, these lines have to be stripped from the sequences beforehand.
The stripped lines are restored after calculation so the class can also
be used as sequenceMatcher.
@DigiLive
Copy link
Collaborator Author

All the renderers but Cli renderers should now respect the ignoreLines option.

@DigiLive
Copy link
Collaborator Author

Please check the output of all renderers carefully with option ignoreLines at all 3 modes.
I'm not sure all produce the desired output.

@DigiLive DigiLive self-assigned this Dec 23, 2020
@DigiLive
Copy link
Collaborator Author

@vendeeglobe Do you mind testing this PR also?

@JBlond
Copy link
Owner

JBlond commented Dec 29, 2020

I'm not sure if this changes anything in the output. I added some empty lines. I tried all 3 options with the same result.

https://i.ibb.co/Sf8zzGG/Bild-2020-12-29-133952.png

@DigiLive
Copy link
Collaborator Author

DigiLive commented Dec 29, 2020

I'm not sure if this changes anything in the output. I added some empty lines. I tried all 3 options with the same result.

https://i.ibb.co/Sf8zzGG/Bild-2020-12-29-133952.png

That's because you added the new lines to a block of lines which is added to version 2 of which the first line isn't blank.
As is now, blocks which contain only blank lines are ignored. E.g. Inserting lines after line 21 of b.txt.

06-01-2021:
I've compared the SideBySide renderer to WinMerge 2.16.6.0 and it seems they ignore blank lines the same way.
(Except for us ignoring the first blank lines of a block and WinMerge ignoring the last blank lines.)

@JBlond
Copy link
Owner

JBlond commented Jan 6, 2021

Sorry for the delay. It works now. I can't tell you why, but it didn't work until I deleted the repo and cloned it refresh.

--- edit

now there only left those comments

lib/jblond/Diff/Renderer/MainRenderer.php, line 100
lib/jblond/Diff/Renderer/SubRendererInterface.php, line 78

DigiLive added 10 commits January 19, 2021 16:04
* Add missing documentation.
* Fixes and reformatting.
* Reduction of cyclomatic complexity.
* Add option `ignoreLines` to control the ability to ignore blank or
  empty lines.
* Add styles of ignored lines to example css.
* Add generator for ignored lines to HTML SideBySide renderer.
* Add `options` property to the Sequence Mather.
* Refactor option `ignoreNewLines` of the Sequence Matcher.
* Add flags to define the level of ignoring blank lines.
* Add tag `ignored` to the opCodes.
* Add method stripLines to Similarity.
* Add default option `ignoreLines` to Diff.
* Add tests for testing option `ignoreLines` of the Sequence Matcher.
* Reformat code.
Instead of defining constants with the same name and values at different
 namespaces, they're now defined at an interface which can be shared
among classes by implementing this interface.
If a used sub renderer, which extends the main renderer, a deprecation
notice will be thrown when the sub renderer is missing method
`generateLinesIgnore`.
Currently the line is commented out, because the method is not required
 until the deprecation period for missing this method is over.
For calculating the similarity ratio when empty/blank lines are
ignored, these lines have to be stripped from the sequences beforehand.
The stripped lines are restored after calculation so the class can also
be used as sequenceMatcher.
@DigiLive
Copy link
Collaborator Author

now there only left those comments

Yes... There's still a cleanup to do once the code it's functionality is approved.
At first it's necessary to carefully inspect the code for bugs or unwanted behavior.

# Conflicts:
#	example/dark-theme.css
#	example/styles.css
#	lib/jblond/Diff/Renderer/Html/Merged.php
@DigiLive
Copy link
Collaborator Author

Ok... I kind of lost track 😅.

Not being allowed to merge remote branch into local (no-ff), I rebased the local branch on the remote.
I assume both branches now contain the changes you've merged earlier today (27-03-2020) and on top of that, my local branch contains the changes I've made in the meanwhile.
Hopefully nothing got broken.

All we had left to do was code cleanup when review was approved?

@JBlond
Copy link
Owner

JBlond commented Apr 1, 2021

Yes, you only wanted to clean up and we are done.

@JBlond JBlond marked this pull request as ready for review October 21, 2021 08:59
@JBlond
Copy link
Owner

JBlond commented Oct 21, 2021

@DigiLive This looks good, There is only a small merge conflict.

@JBlond JBlond merged commit d6b2ef6 into master Oct 26, 2021
@JBlond JBlond deleted the php-diff-82 branch October 26, 2021 18:42
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.

TODO: Ability to ignore blank line changes
2 participants