remove script-local scope

[lacygoill]

I think we can remove the script-local scope all the time. There might be some edge cases where it's needed (and even then I wonder whether it's a Vim bug), but not in the current plugin.

To be able to get rid of s: in the popup options related to the filter and the callback, I also had to remove function(), which is allowed and documented somewhere at :h vim9

Omitting function()

A user defined function can be used as a function reference in an expression without function(). The argument types and return type will then be checked. The function must already have been defined. >

    var Funcref = MyFunction

When using function() the resulting type is "func", a function with any number of arguments and any return type. The function can be defined later.

[lacygoill]

I think we can remove the script-local scope all the time.

The rationale being that all variables and functions declared in a Vim9 script are automatically local to the latter. They are not global like in a legacy script. I think removing s: makes the code a little less verbose, which makes it easier to read.

[yegappan]

Hi, The 'filter' argument for the popup needs a script-local functionand doesn't work with a global function. So I have used ascript-local function for the 'filter' key. Regards,Yegappan On Monday, October 5, 2020, 04:36:32 PM PDT, lacygoill notifications@github.com wrote:

I think we can remove the script-local scope all the time.

The rationale being that all variables and functions declared in a Vim9 script are automatically local to the latter. They are not global like in a legacy script. I think removing s: makes the code a little less verbose, which makes it easier to read.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[lacygoill]

The 'filter' argument for the popup needs a script-local functionand doesn't work with a global function. So I have used ascript-local function for the 'filter' key.

This is unexpected. I know that – sometimes – s: is needed in front of a script-local function name used as a funcref (I think those cases are bugs because they're very inconsistent; I'll report them later), but only when function() is used. If we drop function(), we never need s:. I've checked after applying this patch:

diff --git a/autoload/fileselect.vim b/autoload/fileselect.vim
index 2415c36..2ba0d4a 100644
--- a/autoload/fileselect.vim
+++ b/autoload/fileselect.vim
@@ -87,7 +87,7 @@ enddef

 # Handle the keys typed in the popup menu.
 # Narrow down the displayed names based on the keys typed so far.
-def s:filterNames(id: number, key: string): number
+def s:FilterNames(id: number, key: string): number
   var update_popup: number = 0
   var key_handled: number = 0

@@ -95,7 +95,7 @@ def s:filterNames(id: number, key: string): number
   # timer.
   s:refresh_timer_id->timer_stop()
   if key != "\<Esc>"
-    s:refresh_timer_id = timer_start(1000, function('s:timerCallback'))
+    s:refresh_timer_id = timer_start(1000, TimerCallback)
   endif

   if key == "\<BS>" || key == "\<C-H>"
@@ -223,11 +223,11 @@ def s:processDir(dir_arg: string)

   s:updatePopup()
   if s:pending_dirs->len() > 0
-    s:refresh_timer_id = timer_start(500, function('s:timerCallback'))
+    s:refresh_timer_id = timer_start(500, TimerCallback)
   endif
 enddef

-def s:timerCallback(timer_id: number)
+def s:TimerCallback(timer_id: number)
   s:processDir('')
 enddef

@@ -253,7 +253,7 @@ def fileselect#showMenu(pat_arg: string)
       maxwidth: 60,
       fixed: 1,
       close: "button",
-      filter: function("s:filterNames"),
+      filter: FilterNames,
       callback: EditFile
   }
   popup_winid = popup_menu([], popupAttr)

It removes all function() calls; but FilterNames and TimerCallback still refer to script-local functions. They do not refer to global functions, even though their names is not prefixed with g:.

Then, I've started Vim with no config, except the fileselect plugin:

$ cd /path/to/fileselect; vim -Nu NORC --cmd 'set rtp^=/path/to/fileselect' +'Fileselect'

Then, I've pressed r to filter out all filenames except the README.md file in the popup. Finally, I've pressed Enter to jump to the file: no error is raised. The plugin works as expected:

More generally, we can remove s: anywhere in this plugin, without breaking it, and without losing the script-local scope. Any function or variable which is defined at the script level, is automatically local to the latter. Unless it's prefixed by another scope prefix (e.g. g:).

---

Unrelated to this issue, but there is still an eval string: https://github.com/yegappan/fileselect/blob/da907efc9a82d617b691b56183fec8f4a7f5b040/autoload/fileselect.vim#L172

It could be refactored into a lambda to improve the performance:

s:filelist = s:filelist->map({_, v -> fnamemodify(v, ':p:~:.')})

[lacygoill]

The 'filter' argument for the popup needs a script-local functionand doesn't work with a global function. So I have used ascript-local function for the 'filter' key.

I could be wrong, but I think that when you tried to remove the s: scope, you didn't remove function() nor the quotes around the function name. What matters is the quotes; they need to be removed; then everything works fine, and the code gets more readable, because it's less verbose.

The pitfall you've discovered does not apply only to the 'filter' key of the popup; it also applies to the 'callback' key (but it's less visible; press C-c to trigger it). But it disappears when we drop the quotes and function().

My notes are messy on this subject (I need to rewrite them), but here is an excerpt taken from this link:

actually, what seems to matter is not dropping function(), but dropping the quotes around the function name, which seems to be allowed even when using function()

[lacygoill]

But it disappears when we drop the quotes and function().

Sorry, I meant "when we drop the quotes". Only the quotes matter. function() can stay, but it's useless unless we define a partial.

[yegappan]

Hi,

 > On Thursday, October 8, 2020, 08:12:36 PM PDT, lacygoill notifications@github.com wrote:>>> The 'filter' argument for the popup needs a script-local function and>> doesn't work with a global function. So I have used ascript-local>> function for the 'filter' key.> > This is unexpected. I know that – sometimes – s: is needed in front of> a script-local function name used as a funcref (I think those cases> are bugs because they're very inconsistent; I'll report them later),> but only when function() is used. If we drop function(), we never need> s:. I've checked after applying this patch:> Thanks for the patch.  Your changes work and I have merged them. I was trying to use just FilterNames and it didn't work. I alsotried using function("FilterNames") which also didn't work.In both cases, I removed the s: prefix from the function namewhen defining the function.

 > diff --git a/autoload/fileselect.vim b/autoload/fileselect.vim> index 2415c36..2ba0d4a 100644> --- a/autoload/fileselect.vim> +++ b/autoload/fileselect.vim> @@ -87,7 +87,7 @@ enddef>  >  # Handle the keys typed in the popup menu.>  # Narrow down the displayed names based on the keys typed so far.> -def s:filterNames(id: number, key: string): number> +def s:FilterNames(id: number, key: string): number>    var update_popup: number = 0>    var key_handled: number = 0>  > @@ -95,7 +95,7 @@ def s:filterNames(id: number, key: string): number>    # timer.>    s:refresh_timer_id->timer_stop()>    if key != "\"> -    s:refresh_timer_id = timer_start(1000, function('s:timerCallback'))> +    s:refresh_timer_id = timer_start(1000, TimerCallback)>    endif>  >    if key == "\" || key == "\"> @@ -223,11 +223,11 @@ def s:processDir(dir_arg: string)>  >    s:updatePopup()>    if s:pending_dirs->len() > 0> -    s:refresh_timer_id = timer_start(500, function('s:timerCallback'))> +    s:refresh_timer_id = timer_start(500, TimerCallback)>    endif>  enddef>  > -def s:timerCallback(timer_id: number)> +def s:TimerCallback(timer_id: number)>    s:processDir('')>  enddef>  > @@ -253,7 +253,7 @@ def fileselect#showMenu(pat_arg: string)>        maxwidth: 60,>        fixed: 1,>        close: "button",> -      filter: function("s:filterNames"),> +      filter: FilterNames,>        callback: EditFile>    }>    popup_winid = popup_menu([], popupAttr)>> It removes all function() calls; but FilterNames and TimerCallback> still refer to script-local functions. They do not refer to global> functions, even though their names is not prefixed with g:.> > Then, I've started Vim with no config, except the fileselect plugin:> > $ cd /path/to/fileselect; vim -Nu NORC --cmd 'set rtp^=/path/to/fileselect' +'Fileselect'>> Then, I've pressed r to filter out all filenames except the README.md> file in the popup. Finally, I've pressed Enter to jump to the file: no> error is raised. The plugin works as expected:> > gif> > More generally, we can remove s: anywhere in this plugin, without> breaking it, and without losing the script-local scope. Any function> or variable which is defined at the script level, is automatically> local to the latter. Unless it's prefixed by another scope prefix> (e.g. g:).> I was so used to the scoping rules used in the older Vimscript.I have to get used to the Vim9 scoping rules and syntax.

 > Unrelated to this issue, but there is still an eval string:> https://github.com/yegappan/fileselect/blob/da907efc9a82d617b691b56183fec8f4a7f5b040/autoload/fileselect.vim#L172> > It could be refactored into a lambda to improve the performance:> > s:filelist = s:filelist->map({_, v -> fnamemodify(v, ':p:~:.')})> I have merged this change also. Thanks,Yegappan