Codecov Report

Merging #10291 (38a4f72) into master (aaadb5b) will decrease coverage by 0.00%.
The diff coverage is 77.27%.

❗ Current head 38a4f72 differs from pull request most recent head 84f8188. Consider uploading reports for the commit 84f8188 to get more accurate results

@@            Coverage Diff             @@
##           master   #10291      +/-   ##
==========================================
- Coverage   81.62%   81.62%   -0.01%     
==========================================
  Files         158      158              
  Lines      184737   184912     +175     
  Branches    41768    41822      +54     
==========================================
+ Hits       150797   150938     +141     
- Misses      21452    21463      +11     
- Partials    12488    12511      +23     

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaadb5b...84f8188. Read the comment docs.

Is there a way to set a callback function directly instead of a command, just like we can set callbacks for jobs, timers or popups?

Is there a way to set a callback function directly instead of a command, just like we can set callbacks for jobs,
timers or popups?

Currently you can use only an Ex command to handle an autocmd event.
It is possible to modify this to support a callback function.

What does autocmd_set() do when there already is an autocommand with the same group? When using ":autocmd" it would get added, which probably works best. Having a "delete" flag for autocmd_set() seems strange. Perhaps it's better to use autocmd_add() and autocmd_delete().

@yegappan Currently it is not documented how autocmd_set() behaves when both bufnr and pattern are specified.

I think this PR also addresses #6339.

@yegappan Currently it is not documented how autocmd_set() behaves when both bufnr and pattern are specified.

I have updated the help text to describe how buffer-local autocmds can be added and deleted.

Personally I preferred the previous version where a buffer number could be specified directly without doing any string concatenation like '<buffer=' .. bufnr(mybuf) .. '>'.

The documentation just wasn't clear enough whether the bufnr or pattern option had precedence when both were specified.

I think it would be a lot more readable if we could set the buffer number directly, especially in vim9script.

Personally I preferred the previous version where a buffer number could be specified directly without doing
any string concatenation like '<buffer=' .. bufnr(mybuf) .. '>'.

The documentation just wasn't clear enough whether the bufnr or pattern option had precedence when both
were specified.

I think it would be a lot more readable if we could set the buffer number directly, especially in vim9script.

I have updated the PR to support specifying the buffer number and updated the doc.

I'll leave this open for comments a couple of days.

Nit: "automatic commands" isn't used in the help, only "autocmds" and "autocommands". It's a weird name anyway, but we're stuck with it.

I think the documentation in :h autocmd_delete() is outdated.

In the examples of autocmd_delete() the function is called with the entry 'delete' which doesn't exist.

And what is the purpose of nested or once in autocmd_delete()? Aren't these options only useful for autocmd_add()?

The documentation uses {cmds} or {cmd} to refer to the arguments in autocmd_delete({cmds}) and autocmd_add({cmds}). I think this is a bit confusing since there's already an entry called cmd.

@yegappan can you respond to the comments by @bfrg ?

One common pitfall when adding an autocmd is to forget to wrap it inside a self-clearing augroup.

Bad:

autocmd CursorHold * echomsg 'do something'

Good:

augroup MyGroup
    autocmd!
    autocmd CursorHold * echomsg 'do something'
augroup END

The first snippet is bad, because if for some reason the script is sourced multiple times, there will be a duplicate autocmd:

vim9script
var code = 'autocmd CursorHold * echomsg "do something"'
[code]->writefile('/tmp/script.vim')
source /tmp/script.vim
source /tmp/script.vim
autocmd CursorHold

--- Autocommands ---
CursorHold
    *         echomsg "do something"
              echomsg "do something"

Depending on the cost of the executed command and/or the frequency of the event, this might slow Vim down noticeably, or even cause errors.

Now, here is how we can rewrite the good snippet with the new functions:

vim9script
if exists('#MyGroup')
    autocmd_delete([{group: 'MyGroup', event: '*'}])
endif
autocmd_add([{group: 'MyGroup', event: 'CursorHold', pattern: '*', cmd: "echomsg 'do something'"}])

The whole if exists() block is cumbersome to read/write every single time we need to add an autocmd. Would it make sense for the dictionaries expected by autocmd_add() to support an extra unique boolean option (true by default)?

When unique is true, the autocmd would not be added if it's inside an augroup which already includes the same autocmd. And since unique would be true by default, we could completely omit the previous if exists() block.

@brammool's comment from the mailing list, which hasn't come through to github:

The whole if exists() block is cumbersome to read/write every single time we need to add an autocmd.

Using "silent!" helps though. Here you don't care about errors.

Would it make sense for the dictionaries expected by autocmd_add() to support an extra unique boolean option (true by default)?

When unique is true, the autocmd would not be added if it's inside an augroup which already includes the same autocmd. And since unique would be true by default, we could completely omit the previous if exists() block.

It's probably better to add a "replace" argument: If an autocommand for this group and event already exists, replace it with this one. This works better when someone edits the .vimrc to change the command, sourcing the file again should use the new one, not keep the old one.

autocmd_add([{replace: true, group: 'MyGroup', event: 'CursorHold', pattern: '*', cmd: "echomsg 'do something'"}])

It's probably better to add a "replace" argument: If an autocommand for this group and event already exists, replace it with this one.

I think either unique or replace are both fine (although unique is more descriptive in my mind), as long as the default value is true so we normally don't need to use it, or even know about it! It is very rare that an autocmd should not replace an existing one with the same group and event.

If a group is not specified, then the default group will be used.
[...]
If we make 'replace' the default, then a call to autocmd_add() in a .vimrc
file will remove commands added by a plugin (in the default group) for that event.
A user may not expect this.

IMO, an autocmd should always be added in a named augroup; not in the default one. Unless we're quickly testing some feature.

I was not clear enough in the previous post, sorry. My idea was for unique/replace to be considered only when the autocmd was placed in a named augroup. Because AFAIK the default augroup can't be cleared specifically (right?). So, it wouldn't make sense for autocmd_add() to provide a feature which was not also provided by :autocmd.

And if replace is ignored for the default augroup, then there would be no issue (right?).

The only counter-argument I can find is that it would make replace more complicated to understand. But IMO, doing what the user expects is more important. And to quote :help local-options:

Unfortunately, doing what the user expects is a bit complicated...

Anyway, the mere introduction of replace would already be a big improvement. Making it default for named augroups would be even better, but not that important.

Thank you very much for working on these functions, they're very useful.

For example:

   autocmd_add( ... some group and event ..., command one)
   autocmd_add( ... some group and event ..., command two)

Here the user adds two commands for the same group and event.

I was thinking that the comparison would not be based only on the group and event, but also on the actual command. But maybe that would have been too tricky to implement. Anyway, I'm fine with replace being false by default. I've started using it here, and it seems to work as expected.

There is another issue: it's not easy to use autocmd_add() to install an autocmd listening to several events.

vim9script
autocmd_add([{group: 'SomeGroup', event: 'BufEnter,WinEnter,VimEnter', pattern: '*', cmd: 'echomsg "do something"'}])
autocmd

--- Autocommands ---
SomeGroup  BufEnter
    *         echomsg "do something"

Notice that autocmd_add() has ignored everything after the comma (i.e. WinEnter and VimEnter).

It would be useful for the event key to either accept a comma-separated list of events, or better yet a list of strings. Trying the latter currently gives E730:

vim9script
autocmd_add([{group: 'SomeGroup', event: ['BufEnter', 'WinEnter', 'VimEnter'], pattern: '*', cmd: 'echomsg "do something"'}])

E730: Using a List as a String

And I guess the same limitation applies to a comma-separated list of patterns.

Is there a reason to use a list?

I'm not sure. While it's more verbose, it seems that an explicit list data structure is a better fit for a list of events/patterns than a string, where the list is implicit (we have to mentally split the string when reading the code).

It might also help in some corner cases. For example, in the middle of a comma-separated list of patterns, if one of them starts with a comma, then it needs to be escaped to not be confused with a delimiter:

autocmd WinEnter *.a,\,*.b echomsg 'do something'
                     ^

Here the pattern is meant to match a file whose extension is .a, or a file starting with a comma and whose extension is .b. With an actual list, there would be no need to escape the comma.

Also, the first autocmd in my vimrc listens to a list of events which is specified by a heredoc (i.e. a list of strings, not a string):

var delay_slow_call_events: list<string> =<< trim END
    CursorHold
    InsertEnter
    WinEnter
    CmdWinEnter
    BufWritePost
    QuickFixCmdPost
    TextYankPost
    TextChanged
END
augroup DelaySlowCall
    autocmd!
    execute $'autocmd {delay_slow_call_events->join(",")} * DelaySlowCall()'
    autocmd CmdlineEnter :,/,\?,@ DelaySlowCall()
augroup END

With a string, I would probably write:

[{
    group: 'DelaySlowCall',
    event: delay_slow_call_events->join(','),
    pattern: '*',
    cmd: 'DelaySlowCall()',
}]->autocmd_add()

With a list:

[{
    group: 'DelaySlowCall',
    event: delay_slow_call_events,
    pattern: '*',
    cmd: 'DelaySlowCall()',
}]->autocmd_add()

In the latter, there is no need for ->join(',').

Although, I admit that these are corner cases. Ideally, autocmd_add() would support both a comma-separated list of events/patterns, as well as a list of events/patterns for the values of the event and pattern keys.

Just a comma-separated list would be fine too, I guess. And at least an error, instead of silently ignoring everything after the first comma.

+1 for using lists for lists of things, otherwise there would be no point in having types other than strings (and reinvent a poor man's TCL).