Based on the discussion in #19205 (comment), I took the freedom of clarifying an autoload related paragraph that read a bit too cryptic.
I also wanted to add a note by which Section *52.4* Other mechanisms to use only applied to legacy Vim.
In-fact, I tried something similar in Vim9 through the following script:
vim9script noclear # Note that if you define var did_load inside the if-endif block, then the scope of did_load is confined inside the if-endif block. export var did_load = false echom "Initial did_load: " .. did_load if !did_load command -nargs=* BNRead BufNetRead(<f-args>) map <F19> :call BufNetWrite('something')<CR> did_load = true echom "Final did_load: " .. did_load exe 'au FuncUndefined BufNet* source ' .. expand('<sfile>') finish endif def BufNetRead(a: string) echo 'BufNetRead(' .. string(a) .. ')' # read defality here enddef def BufNetWrite(a: string) echo 'BufNetWrite(' .. string(a) .. ')' # write defality here enddef
and I expected to see
false true true
But I got:
false true false true
which suggests that what is suggested there only apply to legacy vim script.
https://github.com/vim/vim/pull/20833
(1 file)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
Here is my review. (I used Claude Code for assistance, but I've reviewed and verified the contents myself.)
The original paragraphs are indeed terse, so the intent is welcome. The patch
as it stands has one accuracy problem and a number of markup and style issues,
so it cannot go in unchanged.
import autoload "foo.vim"
exported symbols are available through the global autoload namespace.import autoload "/some/path/foo.vim"
exported symbols remain local to the importing script.
What decides this is not whether a path is given, but whether the script sits
under an autoload/ directory. script_name_after_autoload() in
src/scriptfile.c scans the script path for /autoload/, and
handle_import_fname() in src/vim9script.c installs the prefix whenever the
import is an autoload one and that scan succeeds. So
import autoload "/some/path/autoload/foo.vim"
also ends up in the global autoload namespace, which contradicts the second
bullet.
The prose further down gets this right, since it says "defined in a script
outside an autoload/ directory". If the summary stays, it needs the same
qualifier.
:help vim9-reload already covers the whole topic:
The exported functions and variables of an autoload script live in the
global namespace with the autoload prefix. When such a script is sourced
again they are likewise given a clean slate, so an autoload script can be
sourced more than once. A class or enum in an autoload script is an
exception: it cannot be redefined this way and |E1041| is given, because
objects created from the previous definition would keep referring to it.
The user manual is a tutorial and the reference manual is the place for the
rules, so a pointer to |vim9-autoload| would serve the reader better than a
second copy that can drift. Note also that the reference gives the reason for
|E1041|, while the patch only restates that it cannot be redefined.
|runtimepath|, |packpath| and |import| are not tags. The correct forms
are 'runtimepath', 'packpath' and |:import|. The surrounding text
already used 'runtimepath' before this patch.
following |enum|:
<
enum Color
...
endenum
>
and assume that you want to expose it.
> starts an example block and < ends it, so these two are swapped.
The examples in this file are indented with a tab and the block starts with a
trailing > on the preceding text line, as in "private function: >". The new
examples use two spaces and a > on a line of its own, so the section now
mixes both conventions.
Vim help uses two spaces after a sentence. The patch uses one in at least nine
places, for example "until it is actually needed. This is done with", "a
unique script name. Plugin managers often" and "an example. Assume that".
These lines exceed 78 columns:
be deferred until it is actually needed. This is done with the `autoload` keyword.
If the same |enum| as in the previous example is defined in a script outside an
`autoload/` directory and imported using a relative or absolute path, it remains
const my_plugin_path = globpath(&rtp, 'lib/my_typedef.vim')->fnamemodify(':h')
Finally, when a script is imported without `autoload` and without path, like the
"in the `autoload/ directories under" has an unbalanced backtick.
"On Unix systems, one such directories is often ~/.vim/autoload" needs "one
such directory", and has a stray double space before the path.
"errors in getmessage script, are detected only at runtime" has a stray
comma, and "One possible way could be to something like the following" is
missing a word.
The enum example writes white in lower case and the rest capitalised. Both
are legal, see |E1415|, but it reads as a typo.
The same example is introduced as a script that "exports the following enum",
yet the snippet has neither vim9script nor export.
"../lib/getmessage.vim is not loaded or checked at import time" is not quite
right for the path form: handle_import_fname() checks that the file is
readable at import time and gives an error if it is not. Only the contents go
unchecked.
globpath() returns matches separated by newlines, so ->fnamemodify(':h')
breaks as soon as there is more than one match, and it yields an empty string
when there is none. The patch calls the idiom "verbose and not robust" itself.
An example in the user manual is going to be copied, so I would rather not
show this one. The import part is fine, an expression is accepted there.
The PR description says section 52.4 should be marked as legacy only. That
would not be right. The Vim9 equivalent of the did_load guard is documented
under :help vim9-reload:
vim9script noclear
setlocal completefunc=SomeFunc
if exists('*SomeFunc')
finish
endif
The test in the description guards on export var did_load = false, and that
initialiser runs again on every source, which is why the value goes back to
false. The documented idiom guards on the existence of the function instead.
Keeping the rewrite of 52.2 for readability is worthwhile. I would drop the
namespace and |E1041| exposition in favour of a reference to
|vim9-autoload|, drop the globpath() example, and fix the markup and the
style points above. That leaves a clear win without duplicating the reference
manual.
Thanks.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
I updated the PR by addressing the reviewers comments as it follows:
The implications of having a script in an autoload/ folder has been highlighted even more clearly now,
The remark on the effect of re-sourcing a Vim9 script has been removed, and a link to the reference manual has provided as requested (even if prose in the referenced link is somehow "terse"),
Broken links should be fixed by now,
Documentation style has been aligned with the rest of :help guidelines,
I agree, that the example with the globpath is not robust (it was already mentioned in the previous version), but I also agree that a distracted user could just copy and paste the example, despite it may fail in some corner cases. Hence, I completely removed the hint as requested, we can leave users alone to figure out how to import symbols by themselves for this use case,
The review about Section 5.4 is not correct. The differences between the two examples can be easily spotted at first sight, as I explain next:
The example in 52.4 works as it follows:
s:did_load does not exist, so the if block runs: commands and mappings are defined, s:did_load is set to 1, a FuncUndefined autocommand is registered, and then finish exits early. The functions at the bottom of the script are not loaded yet.BufNet* function is actually called): s:did_load exists, so the if block is skipped and the functions are defined.This is a deferred loading mechanism: the heavy part of the script is never loaded unless the user actually invokes the functionality.
The vim9script noclear plus exists('*SomeFunc') pattern, by contrast, is a run-once guard: it prevents re-sourcing from redefining symbols, but it loads everything on the first source. It is not a replacement for 52.4.
Regarding the applicability of the strategy for lazy loading described in 52.4 in Vim9, I voluntarily left it out from the help file because in-spite there is a strong evidence that such a method only applies to vim-legacy, I don't have a formal proof. Nevertheless, I thought it was at least worth mentioning it here.
Note that the reviewer mentioned that he/she used Claude code for the review task. However, the review mentioned in point 5., which suggests the equivalence of the two examples, may have been taken at face value without any deeper verification, which I agree that it is something that could happen when doing AI-assisted work. This is a good reminder that AI tools are helpful for spotting style issues, but technical questions should still be carefully reviewed by human reasoning.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
Thanks for the updates on the other points.
You are right that the two snippets are different mechanisms, and my quote was
a poor choice for that reason.
The conclusion still does not follow, though. The evidence for "52.4 does not
apply to Vim9" is a script that prints false true false true, and that comes
from the guard, not from the mechanism:
export var did_load = false
A declaration is a statement. noclear keeps the item across a re-source, but
the initialiser runs every time, so the value is back to false before the
if is reached. On a re-source scriptfile.c marks the existing script
variables with DI_FLAGS_RELOAD, which is what lets the declaration run again
without E1041.
Guard on the existence of an item rather than on a value, which is what :help vim9-reload does with exists('*SomeFunc'). 52.4 defines no function on the
first source, so the function cannot serve as the marker there, but the command
it does define can. With that, 52.4 keeps working in Vim9, structure unchanged:
vim9script noclear if !exists(':BNRead') command -nargs=* BNRead g:BufNetRead(<f-args>)
exe 'au FuncUndefined BufNet* source ' .. expand('<sfile>') finish endif
def g:BufNetRead(a: string) ... enddef
After the first source only the command exists, the function is not defined
yet. Calling g:BufNetRead() fires FuncUndefined, the script is sourced
again, the guard lets it through, and the function is defined and runs. That is
the same deferred loading 52.4 describes.
So the note would not be correct. What is worth saying instead is that new
Vim9 code has import autoload for this purpose, which is a better thing to
point at than the FuncUndefined trick.
On your closing remark: agreed, technical questions deserve verification rather
than being taken at face value. That is the standard I applied to your test
before writing the review.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
The conclusion still does not follow, though. The evidence for "52.4 does not
apply to Vim9" [...]
I forgot to mention: " ... without using g: items".
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
On 1, "without using globals": that works as well. Same structure as 52.4, one
file, no g: anywhere, with a script-local def:
vim9script noclear if !exists(':BNRead'
)
command -nargs=* BNRead BufNetRead(<f-args>
) exe 'au FuncUndefined BufNet* source ' .. expand('<script>') finish endif def BufNetRead(...args: list<string>) echo 'BufNetRead(' .. string(args) .. ')' enddef
The first source defines only the command. :BNRead abc def then fires
FuncUndefined, the script is sourced again, the def is defined and the call
runs. Checked here with 9.2.
BufNetRead() above is script-local, not global. From :help vim9-scopes:
When using `:function` or `:def` to specify a new function at the
script level in a Vim9 script, the function is local to the script.
Like prefixing "s:" in legacy script.
The only change from 52.4 is that the guard tests the existence of an item
instead of a value, for the reason given in my previous comment. That is also
the counter-example asked for in 2.
3 is fair for the section as it stands. 52.4 is built around a single file, so
import autoload does not fit into it unchanged.
One remark on the additional file, though. A separate script under autoload/
is the normal layout for Vim plugins, and close to all of them are written that
way, so it is the convention rather than an extra cost.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
On 1, "without using globals": that works as well. Same structure as 52.4, one
file, no g: anywhere, with a script-local def:
I can buy it, but mind that you are using a global symbol, whereas 52.4 use a script-local symbol.
One remark on the additional file, though. A separate script under autoload/
is the normal layout for Vim plugins, and close to all of them are written that
way, so it is the convention rather than an extra cost.
But mind what the docs of 52.4 says:
Some may find the use of several files [a](https://vimhelp.org/insert.txt.html#a) hassle and prefer to keep everything
together in one script
I (partially) agree with your point. I also prefer use multiple files, but not in autoload/. I value encapsulation.
But regardless of our own personal preferences, we cannot neglect what is written in the docs or what other users want to do.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
On the global symbol: the marker in my version is the command, and 52.4 defines
that same command.
command -nargs=* BNRead call BufNetRead(<f-args>)
It also defines a global mapping and two global functions, since a function
without a prefix is global in legacy script. My version defines the command and
nothing else global, so its global surface is smaller than 52.4's, not larger.
What does differ is that my guard tests a global name while 52.4 tests a
script-local one, so a command of the same name from elsewhere would fool it.
That is a fair point about the guard, not about the mechanism.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
What does differ is that my guard tests a global name while 52.4 tests a
script-local one,
This is exactly what I meant when I mentioned "globals".
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
I think we can merge this PR now, what do you think? Ping @chrisbra
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
Two formatting points on the new text.
Lines are wrapped earlier than they need to be. The file has textwidth=78 in
its modeline, and the convention is to fill up to it, so a line should not be
broken while the next word still fits, unless the break is intentional. Some
examples, with the width of the line and what the next word would make it:
139 ... an imported script is 65 -> 72
146 ... of an imported script may 66 -> 69
148 keyword. 8 -> 14
152 Nothing in the rest of the script needs to change. 50 -> 59
175 under 'runtimepath'. 20 -> 25
184 ... in 'runtimepath', choose 71 -> 73
There are about 15 such lines in the added paragraphs. Rather than fixing them
by hand, let Vim reflow the paragraph: gqip in Normal mode, gq on a Visual
selection, gqq for a single line. It picks up textwidth=78 from the
modeline, and 'joinspaces' is on by default so the two spaces between sentences
are kept.
By the way, there is still one single space after a full stop left, at line
217. gq will not catch that one, it is mid-line.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
@h-east, I need to be direct here.
I appreciate the formatting suggestions, but I'm concerned about the review process on this PR.
You stated you used an AI tool and "reviewed and verified" the contents, yet your initial analysis contained a fundamental misunderstanding about the difference between lazy loading (52.4) and run-once guards (vim9script noclear). I had to spend significant time explaining this, despite the point being clearly visible at first sight by any human that has some experience with vim script.
Now that the technical discussion is resolved, we're down to line wrapping and spacing.
While valid, feel like a disproportionate focus given the effort already invested.
The core technical content of this PR is sound. The help text is improved. Can we agree on the substantive changes and handle the remaining formatting as a follow-up, or with a quick final commit from my side?
I'm happy to fix the remaining formatting, but I'd like to avoid another round of AI-generated nitpicks that require manual verification.
Let me know if we can move forward on the substance.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
Two formatting points on the new text.
Lines are wrapped earlier than they need to be. The file has
textwidth=78in its modeline, and the convention is to fill up to it, so a line should not be broken while the next word still fits, unless the break is intentional. Some examples, with the width of the line and what the next word would make it:139 ... an imported script is 65 -> 72 146 ... of an imported script may 66 -> 69 148 keyword. 8 -> 14 152 Nothing in the rest of the script needs to change. 50 -> 59 175 under 'runtimepath'. 20 -> 25 184 ... in 'runtimepath', choose 71 -> 73There are about 15 such lines in the added paragraphs. Rather than fixing them by hand, let Vim reflow the paragraph:
gqipin Normal mode,gqon a Visual selection,gqqfor a single line. It picks uptextwidth=78from the modeline, and 'joinspaces' is on by default so the two spaces between sentences are kept.By the way, there is still one single space after a full stop left, at line 217.
gqwill not catch that one, it is mid-line.
All the points have been addressed.
@chrisbra sorry if I am a bit blunt here, but from the evidence emerged during the reviewing process of this PR, I feel in charge to highlight that a community that (facts):
is a community that is kinda losing its way.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
A quick final commit from your side is fine, that is what I was asking for. No
follow-up PR needed.
On "we're down to line wrapping and spacing": of the five inline comments, three
were about content, and you applied them all. autoload import corrected
to import autoload at 169, the sentence about types restored at 153, and the
sentence at 213. A reader copying line 169 as it stood would have got an error.
The wrapping is still open. 14 lines in the added paragraphs break while the
next word would fit, including 139, 146, 148, 175 and 184, the ones I listed by
number. gqip per paragraph handles it.
On the "fundamental misunderstanding": the sentence you quoted from my first
review was about the did_load guard, not a claim that the two examples are
equivalent. The same paragraph identified export var did_load = false running
its initialiser again as the reason your test printed what it did, and said the
documented idiom guards on existence instead. That is
the distinction you then spent several comments explaining back to me. Placing
vim9-reload next to 52.4 invited it, but it was a misreading, not a
misunderstanding on my side. The conclusion held, and I posted a script you can
run to check it.
My first review opened with "(I used Claude Code for assistance, but I've
reviewed and verified the contents myself.)" That was accurate. I check what the
tool produces, and where a claim needs checking I write a script and run it, as
I did here. Humans and tools both get things wrong, as your reading of that
sentence shows. Because a tool is involved I work with extra care, so that the
work is not dismissed for that reason alone. If none of that came through in
this exchange, that is genuinely disappointing.
That is as far as I want to take this. The merge decision is @chrisbra's.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
@chrisbra sorry if I am a bit blunt here, but from the evidence emerged during the reviewing process of this PR, I feel in charge to highlight that a community that (facts):
- Accepts AI-generated reviews without proper verification,
- Misses technical distinctions but insists on formatting,
- Makes contributors chase trivialities after substantive work,
is a community that is kinda losing its way.
It's good to point that out, but you don't necessarily have to fix it, I am happy to do the same while merging it. I am a big boy and can handle this just fine ;) But sometimes I miss a few points and then it is good if someone else has reminded me of what style fixes we need to adjust during merge.
On the final documentation change I think that is fine, thanks both for careful detailed review🙏
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()
- Makes contributors chase trivialities after substantive work,
When a PR touches runtime/doc/*.txt there is a minimum set of rules to
follow, and they are written down. :help help-writing says, among other
things, that help files use two spaces after a sentence and that the modeline
sets 'textwidth'. That is where the review comments come from.
If anything, the substantive work being settled is what makes this the right
moment for the small points, as a final pass.
I give the same comments on every doc PR I review, and people act on them.
If anything here is disruptive, it is turning a documentation review into a
statement about the community.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.![]()