-
Notifications
You must be signed in to change notification settings - Fork 589
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
[Common] Add git conflict marker highlighting #4047
base: master
Are you sure you want to change the base?
[Common] Add git conflict marker highlighting #4047
Conversation
This commit adds patterns to treat git conflict markers as comments in syntax definitions. Goals are 1. improve UX when using ST as git merge tool 2. reduce risk of badly breaking syntax highlighting due to conflict markers
038ae7f
to
7af7f63
Compare
I have some thoughts, not in any order, nor necessarily exhaustive:
|
The only downside would be console messages about "no such context".
Rather rarely caused by injected conflict marker patterns as they are atomic and don't push contexts, but possibly by the way ours/theirs is interrupting normal source code. That's nothing which can be fixed.
Possible
Certainly. |
An alternative location for the Merge syntax would be within Text package, which if disabled causes lots of trouble anyway and is thus less likely to be. This would be justified by conflict markers not being limited to git, but rather being a common unix convention, with regards to |
This commit adds diff3 `|||||||` separator. see: https://www.gnu.org/software/diffutils/manual/diffutils.html#Marking-Conflicts-1
This commit adjusts patterns to optionally support names next to all conflict markers in the same way to comply with diff3's manpage.
Yes, that's what I mean. Agreed that it can't be helped.
|
Still UX is way better then with most syntaxes highlighting those markers as operators, tag punctuation or even illegal. |
This commit... 1. applies more generic text.diff3 scope as those markers are not tight to git 2. renames and moves syntax definition to Packages/Text/Diff3.sublime-syntax
Ideally Diff package seems the more suitable target, but it is not included in Sublime Merge and could also be disabled like Git Formats. |
1c0e745
to
b5da5ec
Compare
Moved Diff3.sublime-syntax into Diff package as it finally feels misplaced in Text package. @dpjohnst : As Diff3 is included by all other syntaxes, Diff package needs to be shipped with Sublime Merge, once this PR gets merged. |
Good. I thought about recommending this, and forgot to post. If I were doing this, I'd make the syntax a functional one called Edit: I guess reusing the same contexts wouldn't work since one has to push and the other can't, but maybe having them in that same file would still be useful. |
Name "Diff3" is inspired by the original unix tool producing output with such tags, like unix tool "diff" produces output with syntax "Diff". Renaming it to Conflict.sublime-syntax would mean to remove reference to diff3, which I don't find ideal. Context We could otherwise follow Diff.sublime-syntax and just put everything in
Scoping it |
How about |
How about adding support for normal output? |
That might work. IIRC, one of the (three!) If that's the route you like, it doesn't need to hold up this PR. There's enough here already that might be controversial. |
As it is planned to add more than just conflicts.
Well, I'd probably start with normal output mode. I have no strong meaning about scope names - just explained their origin. With a look at Diff we could also use Anything which allows them to be uniquely addressed by color schemes might be ok. The question is what defaults we prefer. Should they appear like comments in the first place or just like plain text (which Mariana/Monokai) highlight them, currently. Another question arises. Diff uses |
I'm opposed. I think the scope you already chose is the right one, and Diff should change to match.
Oh, the conflict markers? I would not use I was only talking about if a simple syntax is made whose
Seems fine. No strong opinions. I've changed my mind on this a bunch of times with no obvious right answer. |
Implements https://www.gnu.org/software/diffutils/manual/diffutils.html#An-Example-of-diff3-Normal-Format Syntax tolerates leading whitespace in case it is to be embedded into Markdown's fenced code blocks or any other possibly indented contexts. Patterns should be strict enough for risk of false positives to be acceptable low.
Apply same syntax specific settings as for diff syntax.
... to align it with main scope Diff.sublime-syntax uses.
Test files should start with |
This PR ...
adds
Git Formats/Git Merge.sublime-settingsText/Diff3.sublime-syntax with simple and restrictive patterns for git conflict markers.They explicitly do not push contexts to avoid interfering with interpolations or any other advanced syntax features.
includes those patterns as sort of comments in all syntax bundled definitions.
Comments are included in most contexts in all syntax definitions, thus conflict markers should be matched properly with high chances as well.
includes merge conflict patterns into known multi-line strings or heredocs.
Goals
Motivation
Remarks
With this PR all syntax definitions depend on Git Merge.sublime-syntax and thus require Git Formats package to be enabled.
Pro: Single file to maintain, simple re-usable context for all syntaxes
Contra: inter-package dependency
Alternative: duplicate
conflict
context into all (about 40) syntax definitions :/Initial state of this PR provides a "works in most cases" quality based on existing comments/strings contexts. There may be some situations left, in which conflict markers are not yet detected. Support can however easily be improved in future.
Some syntaxes such as Markdown use
===
to highlight headings, which still may cause syntax highlighting issues caused by unavoidable ambiguities. So this PR can provide "the base we can" quality.This PR excludes changes to ShellScript to avoid conflicts with [ShellScript] Add ZSH #4024. Further action depends on which PR is merged first - if at all.