-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add --gitsigns option (#3404)
#3407
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
base: master
Are you sure you want to change the base?
Add --gitsigns option (#3404)
#3407
Conversation
5ad42cf to
02dbe87
Compare
|
@sand4rt take a look and tell me what you think :) |
|
Hi! What is the problem with the default signs? If they are problematic somehow it's probably better to fix the problems they have than to add complexity to the code and for users. |
Hi @Enselic , This PR will just add This is how gitsigns looks with the modern preset( The whole feature is about 100 lines, 90% of those +1000 lines are just tests :) |
|
Ok, let's start with the first question. What is the problem with the default signs? |
They are not the same as other terminal apps like gitsigns.nvim |
|
Why not make gitsigns configurable instead of bat? |
It is configurable in gitsigns, but i find the line-based indicators much clearer and easier to parse visually. In my opinion they reduce visual noise compared to the current symbols, and make diffs easier to scan. Popular editors like VSCode and JetBrains IDEs also use line markers as the default. |
Hi @Enselic , what is the problem with the default cat binary that comes with almost all UNIX based systems ? What it the problem with the default signs ? OUTDATED, we aren't in 2015 anymore when SublimeText2 was the most popular text editor(all the respect for SublimeText, still one of the best text editors), at that time, the default gitsigns was:
Those aren't the "default" anymore, with the new era of text editors/IDE, VSCode and all its fork(Cursor ...), Intellij IDE, Neovim with gitsigns.nvim plugin, all are using new gitsigns:
With this PR, we will be able to:
Now tell me, which one is easier to get the git changes from ? classic preset or the modern one used by all modern text editors/IDE ? PS: |
|
Sorry for being annoying. I just needed to be convinced this was a worthwhile thing to change, and now you have convinced me. Thank you for taking the time to do so. I (or some other maintainer) will look at the details later when I get the time. |
| theme | ||
| .scopes | ||
| .iter() | ||
| .find(|item| item.scope.eq(&scope_selectors)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scope selectors from the theme generally shouldn't be expected to exactly match a scope, but instead use scope selector matching logic to determine which theme rules match and which has the highest precedence... Probably it doesn't matter too much here, but I would prefer consistency with how highlighting works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like I missed this reply earlier, sorry about that. I meant that the tmTheme may choose to have rules with less specific scope selectors like markup which would match markup.deleted, markup.changed etc. or even multiple rules could theoretically match, and whose styles would get applied/merged together etc. based on precedence order. So my idea was to use proper scope selector logic instead of checking for string equality, but probably it isn't going to be necessary in practice, because, like you pointed out, most themes have these specific rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but probably it isn't going to be necessary in practice, because, like you pointed out, most themes have these specific rules
Sorry for late response, do you want/suggest using ScopeSelector::does_match instead because i got confused here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer it, yes please 👍
| --gitsigns <gitsigns> | ||
| Set git `added`, `modified`, `removed-above`, `removed-below` signs. (default: classic) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be great if we could also update the auto completion scripts please, so they would suggest the new argument and it's possible values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry i didn't get the point here, can you explain more please ? What do you mean by "update the auto completion scripts" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some scripts which are used for completions in various shells like bash, fish, zsh. When typing bat --t and pressing the tab key, it will automatically finish writing --theme for example. So when adding new command line options like --gitsigns, it would be nice to ensure the completion scripts know to offer that as a suggestion. If it's too much hassle, don't worry, we can always do it later. I think I got Copilot or another LLM AI to help me last time I needed to do it because I don't know all the various shell syntaxes.




See #3404