GNU bug report logs - #79038
[PATCH] Add support for Vi/Vim modelines.

Previous Next

To reply to this bug, email your comments to 79038 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: James Youngman <james <at> youngman.org>
To: bug-gnu-emacs <at> gnu.org
Cc: James Youngman <james <at> youngman.org>, James Youngman <youngman <at> google.com>
Subject: [PATCH] Add support for Vi/Vim modelines.
Date: Thu, 17 Jul 2025 08:44:27 +0100

[Message part 1 (text/plain, inline)]

I'd like to contribute support for Vi/Vim modelines.  These are
somewhat similar to Emacs' local variables.  The basic idea is that
this makes it easier to collaborate with a software author who uses Vi
modelines to correctly set up their editor, and expects others to use
consistent settings.

This change should be covered by the copyright assignment my employer
(Google) already made for GNU Emacs.

This is my first Emacs patch, so I apologise for any misunderstandings
or omissions.  The patch itself is attached, but here is the ChangeLog
entry:

    Add support for Vi/Vim modelines.

    * lisp/emulation/vimvars.el: New file, implementing support for Vi
      modelines.
    * test/lisp/emulation/vimvars-tests.el: New file; tests for this
      feature.
    * doc/emacs/custom.texi(Vimvars): Describe this feature.
    * doc/misc/viper.texi(Modeline): Mention this feature.
    * etc/NEWS: Mention this change.

[0001-Add-support-for-Vi-Vim-modelines.patch (text/x-patch, attachment)]

Message #8 received at 79038 <at> debbugs.gnu.org (full text, mbox):

From: James Youngman <james <at> youngman.org>
To: 79038 <at> debbugs.gnu.org
Subject: Patch update (fixing "make check")
Date: Thu, 17 Jul 2025 17:03:26 +0100

[Message part 1 (text/plain, inline)]

This additional change is needed in order not to break "make check".

[0002-Fix-docstrings-requires-avoid-deprecated-functions-i.patch (text/x-patch, attachment)]

Message #15 received at 79038 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: James Youngman <james <at> youngman.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 79038 <at> debbugs.gnu.org, youngman <at> google.com
Subject: Re: [PATCH] Add support for Vi/Vim modelines.
Date: Sat, 19 Jul 2025 13:26:29 +0300

> From: James Youngman <james <at> youngman.org>
> Date: Thu, 17 Jul 2025 08:44:27 +0100
> Cc: James Youngman <james <at> youngman.org>, James Youngman <youngman <at> google.com>
> 
> I'd like to contribute support for Vi/Vim modelines.  These are
> somewhat similar to Emacs' local variables.  The basic idea is that
> this makes it easier to collaborate with a software author who uses Vi
> modelines to correctly set up their editor, and expects others to use
> consistent settings.
> 
> This change should be covered by the copyright assignment my employer
> (Google) already made for GNU Emacs.
> 
> This is my first Emacs patch, so I apologise for any misunderstandings
> or omissions.  The patch itself is attached, but here is the ChangeLog
> entry:
> 
>     Add support for Vi/Vim modelines.
> 
>     * lisp/emulation/vimvars.el: New file, implementing support for Vi
>       modelines.
>     * test/lisp/emulation/vimvars-tests.el: New file; tests for this
>       feature.
>     * doc/emacs/custom.texi(Vimvars): Describe this feature.
>     * doc/misc/viper.texi(Modeline): Mention this feature.
>     * etc/NEWS: Mention this change.

Thanks, please see the comments below.

> * lisp/emulation/vimvars.el: New file, implementing support for Vi
>   modelines.
> * test/lisp/emulation/vimvars-tests.el: New file; tests for this
>   feature.
> * doc/emacs/custom.texi(Vimvars): Describe this feature.
> * doc/misc/viper.texi(Modeline): Mention this feature.
> * etc/NEWS: Mention this change.

In your future revisions of the patch, please mention the bug number.

> ---
>  doc/emacs/custom.texi                |  24 ++
>  doc/misc/viper.texi                  |  16 +
>  etc/NEWS                             |   6 +
>  lisp/emulation/vimvars.el            | 222 ++++++++++++++
>  test/lisp/emulation/vimvars-tests.el | 428 +++++++++++++++++++++++++++
>  5 files changed, 696 insertions(+)
>  create mode 100644 lisp/emulation/vimvars.el
>  create mode 100644 test/lisp/emulation/vimvars-tests.el

Hmm.. I'm not sure we want to advertise this feature in custom.texi.
It's related to Vi and its emulations, so viper.texi seems like the
right place for it.

Stefan, WDYT about this?  Maybe this is better suited to be an ELPA
package?

> diff --git a/doc/emacs/custom.texi b/doc/emacs/custom.texi
> index 9b545b9fd0a..bfff00cc8f1 100644
> --- a/doc/emacs/custom.texi
> +++ b/doc/emacs/custom.texi
> @@ -777,6 +777,7 @@ Variables
>  * Directory Variables:: How variable values can be specified by directory.
>  * Connection Variables:: Variables which are valid for buffers with a
>                             remote default directory.
> +* Vimvars::             Support for Vi/Vim modelines.
>  @end menu

But if we do eventually decide to describe this in the Emacs user
manual, the @detailmenu menu in emacs.texi should also be updated.

> +The Vi editor (and many of its clones and implementations, including
> +Vim) supports a feature like file local variables. Emacs can optionally
> +support these, like this:                        ^^

Our convention is to leave 2 space between sentences.

> +@cindex risky variable
> +The Vim modeline setting @code{makeprg} is translated to
> +@code{compile-command}, but this is not treated as an unsafe local
> +variable.

I don't understand the purpose of the @cindex entry here with such a
general text.  I also don't understand why makeprg is not treated as a
risky variable.  can you explain?

> +This has an effect similar to adding @samp{set modeline} to
> +@file{~/.vimrc}.

I think "This" here alludes to the @lisp example, not to the text
after it.  So I think rearranging the text is in order.

> ++++
> +** Support for Vim/Vi modelines

Heading lines in NEWS should end in a period.

> diff --git a/lisp/emulation/vimvars.el b/lisp/emulation/vimvars.el
> new file mode 100644
> index 00000000000..50b56cc6912
> --- /dev/null
> +++ b/lisp/emulation/vimvars.el
> @@ -0,0 +1,222 @@
> +;;; vimvars.el --- Emacs support for Vi modelines
> +
> +;; Copyright (C) 2010,2011,2025 Free Software Foundation, Inc.
> +
> +;; Author: James Youngman <james <at> youngman.org>
> +;; Maintainer: James Youngman <james <at> youngman.org>
> +;; Keywords: local-variables, vi, vim, emulations
> +
> +;; vimvars is free software: you can redistribute it and/or modify
> +;; it under the terms of the GNU General Public License as published by
> +;; the Free Software Foundation, either version 3 of the License, or
> +;; (at your option) any later version.
> +
> +;; vimvars is distributed in the hope that it will be useful,
> +;; but WITHOUT ANY WARRANTY; without even the implied warranty of
> +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;; GNU General Public License for more details.
> +
> +;; You should have received a copy of the GNU General Public License
> +;; along with vimvars.  If not, see <http://www.gnu.org/licenses/>.

This preamble should be identical to the other files in the Emacs
source tree.

> +(defcustom vimvars-check-lines 5
> +  "The number of lines in the top or bottom of a file that we will search for Vim settings (Vim itself checks 5)."

The first line of a doc string should be a single sentence shorter
than 80 characters.

> +(defcustom vimvars-ignore-mode-line-if-local-variables-exist t
> +  "If non-nil, Vim mode lines are ignored in files that have Emacs local variables."

Likewise.

Message #18 received at 79038 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: James Youngman <james <at> youngman.org>
Cc: 79038 <at> debbugs.gnu.org
Subject: Re: bug#79038: Patch update (fixing "make check")
Date: Sat, 19 Jul 2025 13:28:39 +0300

> From: James Youngman <james <at> youngman.org>
> Date: Thu, 17 Jul 2025 17:03:26 +0100
> 
> +(require 'simple)                       ; needed for read-only-mode

simple.el is preloaded, so this shouldn't be needed.

> +(require 'cc-bytecomp)                  ; needed for cc-require
> +(cc-require 'cc-vars)                   ; we use c-basic-offset.

Which parts of vimvars need these?  If you need this for the tests,
then the proper place to have these is in vimvars-tests.el, not here.

Message #21 received at 79038 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: James Youngman <james <at> youngman.org>
Cc: 79038 <at> debbugs.gnu.org
Subject: Re: bug#79038: [PATCH] Add support for Vi/Vim modelines.
Date: Sat, 19 Jul 2025 12:39:45 -0400

Regarding Eli's question about keeping it as an ELPA package, I don't
have a strong opinion either way (I'm leaning towards keeping it as
a separate package since nothing in Emacs depends on it, but it also
depends on how you/we plan to maintain it in the future).
I'd be happy to add it to GNU ELPA.

Some comments below.


        Stefan


> @@ -777,6 +777,7 @@ Variables
>  * Directory Variables:: How variable values can be specified by directory.
>  * Connection Variables:: Variables which are valid for buffers with a
>                             remote default directory.
> +* Vimvars::             Support for Vi/Vim modelines.
>  @end menu

How 'bout placing it next to the doc of EditorConfig?

> +@lisp
> +(require 'vimvars)
> +(add-hook 'find-file-hook 'vimvars-obey-vim-modeline)
> +@end lisp

I'd rather make it a global `vimvars-mode`, like `editorconfig-mode`.
Especially since hooking into `find-file-hook` might not be the best way
to go about it, so using a minor mode will make it easy to change
the implementation without forcing users to update their init files.

Regarding "the best way": `editorconfig-mode` and Emacs' own
`.dir-locals.el` support uses `hack-dir-local-get-variables-functions`,
so maybe you could use that.  It's probably not the *best* way, because
it's been designed for dir-local rather than file-local vars, but the
upside is that it naturally gives it lower precedence than
file-local settings.

> --- a/doc/misc/viper.texi
> +++ b/doc/misc/viper.texi
> @@ -1610,6 +1610,7 @@ Customization
>  * Packages that Change Keymaps:: How to deal with such beasts.
>  * Viper Specials::               Special Viper commands.
>  * Vi Macros::                    How to do Vi style macros.
> +* Modeline::                     Emulation for Vi modelines.
>  @end menu
>  
>  @node Rudimentary Changes
> @@ -2993,6 +2994,21 @@ Vi Macros
>  currently defined.  To see all macros along with their definitions, type
>  @kbd{M-x viper-describe-kbd-macros}.
>  
> +@node Modeline
> +@section Emulation for Vi modelines
> +
> +You can enable limited support for Vi modelines, with or without Viper,
> +with @code{vimvars}:
> +
> +@findex vimvars-obey-vim-modeline
> +@lisp
> +(require 'vimvars)
> +(add-hook 'find-file-hook 'vimvars-obey-vim-modeline)
> +@end lisp

That seems redundant with the previous section in `custom.texi`.
Pick one place and add a reference to it from the other place?

> +;;; Code:
> +
> +;;; We say "Vim" here for consistency with Emacs documentation (which
> +;;; uses "Vi"), instead of "VIM" which is what that editor's
> +;;; documentation uses as its name.

These should use ";;" rather than ";;;" (which is reserved for headings).

> +(defcustom vimvars-enabled t
> +  "If nil, Vim mode lines will be ignored."
> +  :version "31.1"
> +  :type 'boolean
> +  :group 'vimvars)

The `:group` arg is redundant (same for all the other `defucstom`s in
the file).

> +(make-variable-buffer-local 'vimvars-enabled)

Why?

> +(defcustom vimvars-check-lines 5
> +  "The number of lines in the top or bottom of a file that we will search for Vim settings (Vim itself checks 5)."
> +  :version "31.1"
> +  :type 'integer
> +  :group 'vimvars)

I think the compiler should emit a warning for this too-long-line docstring.

> +(defun vimvars--accept-tag (leader tag)
> +  "Return non-nil if LEADER followed by TAG should be accepted as a modeline."
> +  (cond
> +   ((equal "vim" tag) t)
> +   ((equal "vi" tag) t)
> +   ;; Accept "ex:" only when it is not at the beginning of a line.
> +   ((equal "ex" tag) (not (equal 0 (length leader))))
> +   (t nil)))

I think you can remove this function and tighten the regexp instead.

> +  (while (re-search-forward
> +          " *\\([^= ]+\\)\\(=\\([^ :]+\\)\\)?" settings-end t)
> +    (let ((variable (vimvars--expand-option-name (match-string 1))))
> +      (if (match-string 3)
> +          (vimvars--assign variable (match-string 3))
> +        (vimvars--enable-feature variable))))

(match-end 3) or (match-beginning 3) would work as well (as a boolean
test) as (match-string 3) but without allocating a new string object.

> +(defun vimvars--set-indent (indent)
> +  "Set the amount of indentation caused by tab to INDENT in a mode-aware way."
> +  (when (equal major-mode 'c-mode) (setq c-basic-offset indent)))

Check `editorconfig-indent-size-vars`; it would be nice to share code here.
Recently the same need showed up in yet some other package, tho I can't
remember where right now.

> +(defun vimvars--expand-option-name (option)
> +  "Expand the abbreviated Vim :set variable OPTION to its full name."
> +  (let ((expansion
> +         (assoc option
> +                '(("ro" "readonly")
> +                  ("sts" "softtabstop")
> +                  ("sw" "shiftwidth")
> +                  ("ts" "tabstop")
> +                  ("tw" "textwidth")))))
> +    (if expansion (cadr expansion) option)))

You can shorten the `if` to (or (cadr expansion) option).

> +;;; Not supported:
> +;;; comments/com (comment leader), because it's not language-specific in Vim.

";;;" => ";;".  Maybe also move it into the function?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.