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

Add syntax highlight code for ignore files #3104

Closed
wants to merge 1 commit into from
Closed

Add syntax highlight code for ignore files #3104

wants to merge 1 commit into from

Conversation

taconi
Copy link
Contributor

@taconi taconi commented Dec 28, 2023

Adding ignore file support by default so you don't need to install the language-ignore plugin.

image

@Andriamanitra
Copy link
Contributor

I like the idea but I would reconsider the syntax groups. The current colors seem a bit weird because files are not statements, wildcards are not identifiers, and negated files are definitely not errors.

Screenshots

Using the default config:
image

Using my current config:
image

Maybe using diff-added and diff-deleted colors would make more sense?

- error: "^\\!.*" # negated

- special: '/'
- identifier: '!|\*|\[.*]'
Copy link
Contributor

@Andriamanitra Andriamanitra Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- identifier: '!|\*|\[.*]'
- symbol.operator: '^!'
- identifier: '\?|\*|\[.*?\]'
- constant.specialChar: "\\\\."
  • ! should only match when it's the first character on the line (exclamation mark in the middle of a file name has no special meaning)
  • ? in .gitignore matches any single character except backslash
  • The regex .* is greedy so it wouldn't work if you have something like dir[a-z]/file[a-z]. Changing it to .*? makes it non-greedy so it only matches until the next ].
  • Add highlighting for escaped characters like \?

Copy link
Contributor Author

@taconi taconi Jan 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on your suggestions I thought of something like this, what do you think?

---
filetype: ignore

detect:
  filename: .*ignore$

rules:
  - statement: .*$           # files
  - constant.string: .*/$    # diretories
  - diff-deleted: '^!.*'    # negated

  - special: /
  - identifier: '^!|\?|\*|\[.*?]|\\.'

  - comment: '#.*$'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the colors strange?

Copy link
Contributor

@Andriamanitra Andriamanitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rules seem to work alright for common patterns but some of the weirder cases are not highlighted correctly.

@taconi
Copy link
Contributor Author

taconi commented Jan 14, 2024

[...] files are not statements, wildcards are not identifiers, and negated files are definitely not errors.
Maybe using diff-added and diff-deleted colors would make more sense?

The relationship was based on colors because of the existing identifiers I did not find any related to the ignore file patterns. diff-added and diff-deleted are also not nominally related.
I believe one thing that might bother you is that some color schemes add a background color to the error handler, so I don't see any problem with switching to diff-deleted.

@taconi taconi requested a review from Andriamanitra March 20, 2024 12:55
Copy link
Contributor

@Andriamanitra Andriamanitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you commit the changes from #3104 (comment) it looks good to me

@taconi
Copy link
Contributor Author

taconi commented Mar 20, 2024

If you commit the changes from #3104 (comment) it looks good to me

@Andriamanitra Applied

runtime/syntax/ignore.yaml Outdated Show resolved Hide resolved
@taconi taconi closed this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants