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

[Diff] Organize contexts, add tests, support Git patch emails #4111

Open
wants to merge 124 commits into
base: master
Choose a base branch
from

Conversation

michaelblyons
Copy link
Collaborator

@michaelblyons michaelblyons commented Dec 7, 2024

This started out as an attempt to use parts of an email syntax I wrote to support front matter in Git patch emails. It kind of ballooned after that to include a big refactor of the base Diff syntax according to the GNU patch spec.

I would not claim this is perfect, but I'd like some other eyes on it. Edit: Much improved. Thanks to the reviewers!

Notable changes

  • Lots of documentation links
  • Lots of new tests, mostly pulled from relevant documentation
  • Some scope tweaking, but preserving the stacking from [Diff] Make scopes consistent (backwards compatible) #2178 for backwards-compatibility
  • New syntax for Git Diff emails (git am and git format-patch)
  • New support for
    • Diff3 formats
    • Conflict sections
    • "Combined" unified diffs, only in Git Diff

Potentially controversial

  • This PR conflicts somewhat with [Common] Add git conflict marker highlighting #4047, but I think that PR also has great merits. I made a "conflict" syntax separate from the diff syntax, but if other people have strong feelings about conflict markers being part of Diff.sublime-syntax, I'd consider trying to bring it in. Edit: I pulled it back into main Diff
  • When the normal Diff syntax detects the "magic" first line, it delegates out to Git Diff instead. Edit: This now happens in a way that doesn't add syntax bloat.
  • I use storage.modifier to distinguish between the two Git file modes (namely 644 and 755)
  • I use markup scopes to color the ++++---- and similar, even though they're not actually markup.
  • I never know what scopes to use for dates.
  • I cheated by delegating users/emails out to Mailmap.
  • Whatever you noticed. 😛

Requested changes

  • Fix unit tests
  • More whitespace
  • Move variables to EOF
  • Move boring contexts to bottom of contexts
  • More named contexts to support inheritance
  • Investigate i10n
    • I haven't found any equivalents in the German po file for stuff I've added scopes to. If someone knows differently, please provide examples.
  • Simplify line endings
  • Base syntax for Diff and Git Diff.

Also include their file modes

[Diff] Change file mode highlighting

[Diff] Loosen file mode match
Includes the Git Commit Message stuff, because that's what it is.
[Diff] Empty file hash
Technically, this is called the "signature."
1. introduce named contexts for content blocks
2. re-include `diff3-edit`
3. add some tests
This commit...

1. scopes `,` in ranges `punctuation.separator.range` as numbers before and
   after denote beginning and end of modified content range, but no sequence
   of arbitrary numbers.

2. scopes diff3 hunks `meta.hunk`

3. scope whole diff3 change markers `meta.range` to achieve same segmentation
   as for `@@ ... @@` range markers in unified diffs.

4. add diff3 modifications to toc-list

   Scope change markers `meta.toc-list`.
   Exclude eol to correctly separate them in symbol list.
@michaelblyons

This comment was marked as resolved.

@deathaxe

This comment was marked as resolved.

@michaelblyons

This comment was marked as resolved.

@deathaxe

This comment was marked as resolved.

@michaelblyons
Copy link
Collaborator Author

michaelblyons commented Dec 18, 2024

I've brought Conflict.sublime-syntax back in, along with a conservative side-by-side highlighter. A more risky side-by-side syntax at michaelblyons#10

@michaelblyons

This comment was marked as resolved.

@jrappen
Copy link
Contributor

jrappen commented Dec 20, 2024

let us know when/if you need any review or feedback 😁

@michaelblyons
Copy link
Collaborator Author

let us know when/if you need any review or feedback 😁

Haha. Sorry if you're flooded in notifications. The PR is a lot better after you and DeathAxe's reviews. I think if you're looking for something, check my PRs to this PR (michaelblyons#11 and michaelblyons#10) and decide if you care about any of the things in the "controversial" heading in OP.

Thanks again.

@jrappen
Copy link
Contributor

jrappen commented Dec 22, 2024

Maybe you could add an empty line between meta patterns (https://www.sublimetext.com/docs/syntax.html#meta-patterns) and the rest in your named contexts.

Compare:

contexts:
  name:
    - meta_include_prototype: false
    - meta_scope: one
    - meta_content_scope: two
    - include: three
    - include: four

vs.

contexts:
  name:
    - meta_include_prototype: false
    - meta_scope: one
    - meta_content_scope: two

    - include: three
    - include: four

Makes it easier to read.

@jrappen
Copy link
Contributor

jrappen commented Dec 22, 2024

On a related note, when applying multiple scopes:

Compare:

contexts:
  name:
    - meta_include_prototype: false
    - meta_scope: one two three
    - meta_content_scope: four
    - include: five
    - include: six

vs.

contexts:
  name:
    - meta_include_prototype: false
    - meta_scope: >-
        one
        two
        three
    - meta_content_scope: four

    - include: five
    - include: six

Makes it easier to read and makes for easier to understand diffs in the git history of this repo.

@deathaxe
Copy link
Collaborator

Empty lines between contexts is ok, but starting arbritary emptly lines between patterns is too much.

@michaelblyons
Copy link
Collaborator Author

There are places where I have considered newlines in stacked scopes, but I am not planning to add an empty line between meta and includes. PackageDev scoping is sufficient to make that distinction very obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Syntax significant T: enhancement Improvement of existing language features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants