Re: [vim/vim] Refined autoload mechanism explanation (PR #20833)

h-east left a comment (vim/vim#20833)

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.

## The summary of the two forms uses the wrong deciding factor

> - `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.

## Most of this is already in vim9.txt

`: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.

## Broken help links

`|runtimepath|`, `|packpath|` and `|import|` are not tags. The correct forms
are `'runtimepath'`, `'packpath'` and `|:import|`. The surrounding text
already used `'runtimepath'` before this patch.

## The enum example has its block markers reversed

	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.

## Example style does not match the file

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.

## Documentation style

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

## Smaller points

"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.

## The globpath() recipe

`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.

## About the planned note on 52.4

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.

## Suggested scope

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 or view it on GitHub:
https://github.com/vim/vim/pull/20833#issuecomment-5079552016
You are receiving this because you are subscribed to this thread.

Message ID: <vim/vim/pull/20833/[email protected]>

-- 
-- 
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php

--- 
You received this message because you are subscribed to the Google Groups "vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To view this discussion visit https://groups.google.com/d/msgid/vim_dev/vim/vim/pull/20833/c5079552016%40github.com.