Check for diff3-style merge conflict artifacts.#586
Check for diff3-style merge conflict artifacts.#586christhekeele wants to merge 2 commits intopre-commit:mainfrom
Conversation
| CONFLICT_PATTERNS = [ | ||
| b'<<<<<<< ', | ||
| b'======= ', | ||
| b'||||||| ', |
There was a problem hiding this comment.
would you like to add a failing test for this? the testsuite passes because that particular string is part of a larger string which contains the other strings
There was a problem hiding this comment.
Sure! Think I can get to that later tonight
There was a problem hiding this comment.
@asottile I gave this a go, but am encountering some friction with running tests locally (test dependencies are missing from requirements-dev.test, at least ruamel.yaml, and I'm getting unexplicable tmpfile errors). The build is also telling me that there is something incorrect about either my understanding of the how the tests work. So not mergable yet, and I'm not pursuing for now, but leaving this open for others!
There was a problem hiding this comment.
I think all you need to do is add to this list: https://github.com/pre-commit/pre-commit-hooks/pull/586/files#diff-6224b62678ea600729955336d2e3f48b0c90be48e8ea57dbefe54e7c00131532R106
are you using tox when testing locally?
There was a problem hiding this comment.
A bit confused there, that commit... is where I added to that list? Then CI fails on this test that I'd expect to succeed.
I've never used tox before but I figured it out (tho still have odd tmpfile-caused test failures). Some dev-instruction that you should be using it, and how to use, may help with future contributors.
Speaking of which, thanks for the help!
There was a problem hiding this comment.
yeah you shouldn't need to touch that test -- it's testing a different behaviour (when --assume-in-merge is passed it should check for those strings) -- I think you can revert your test changes and just add to the tests on line 106
618e847 to
0c6a81a
Compare
I noticed that the test suite checks for
diff3-style merge conflicts, but the actual hook does not. Seems like an oversight?