Introduce TermListPatcher and AliasGroupListPatcher#125
Conversation
Benestar
force-pushed
the
termlistpatcher
branch
from
35315f6 to
cbeea5c
Compare
| @@ -18,9 +19,14 @@ | |||
| class ItemPatcher implements EntityPatcherStrategy { | |||
There was a problem hiding this comment.
Uh, this is weird in my opinion. Item and Property are guaranteed to have a Fingerprint. We do not win anything by using the smaller patchers here.
Benestar
force-pushed
the
termlistpatcher
branch
from
cbeea5c to
a619d48
Compare
|
I removed the questioned commit from the chain. |
| new AliasGroup( 'en', array( 'foo', 'bar' ) ) | ||
| ) ), | ||
| new Diff( array( | ||
| 'en' => new Diff( array( new DiffOpChange( 'bar', 'baz' ) ) ) |
There was a problem hiding this comment.
This looks like a valid change. Why should this be a no-op? Especially since the very similar change below works.
There was a problem hiding this comment.
If I understand it correctly, that is basically what wmde/Diff#65 is about. I just wanted to test the current behaviour but we should change this test if the other pull request gets merged.
I didn't yet completely understand what the $isAssociative is about. Is it just the array of diffs that can be associative or linear? Anyways, it would perhaps be nice to do that in the actual test so that we don't have too much duplication.
Any idea how to fix that? Is it possible to disable the linting per file/method? |
The diffs as well. See the first 3 sections in "usage" at https://github.com/wmde/Diff#usage |
Sounds good. I'm guessing this was not done so far as we actually had no need to expose those things. I don't actually recall the reason though. |
thiemowmde
force-pushed
the
termlistpatcher
branch
from
a619d48 to
9f8471d
Compare
thiemowmde
changed the title
thiemowmde
force-pushed
the
termlistpatcher
branch
3 times, most recently
from
583a06e to
72cd4d7
Compare
|
Rebased again, on top of #120 and #131. All this patch now does is splitting the existing implementation (now without reverting anything) and adding tests. The only relevant detail I had to change in the tests was the one #131 fixed (see #125 (comment)). I also added a few more test cases with the "associative" flag set to true and false. From my point of view this is ready to be merged. Open questions (for other patches, should not block this one):
|
thiemowmde
changed the title
|
Needs manual rebase |
thiemowmde
force-pushed
the
termlistpatcher
branch
from
72cd4d7 to
bc8da99
Compare
4 participants
This pull request is mainly about moving code around and creating a lot of test cases.
It consists of three commits that can be reviewed individually.
After this pull request, FingerprintPatcher is unused and can be removed. We have to make sure that all test cases in FingerprintPatcherTest are also present in TermListPatcherTest and AliasGroupListPatcher test.
To make the new classes actually useful, we have to move them out of the Internal namespace and make them package public.