GNU bug report logs - #32849
26.1; xref-marker-ring-length user option doesn't have setter

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 32849 in the body.
You can then email your comments to 32849 AT debbugs.gnu.org in the normal way.

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

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

From: Allen Li <darkfeline <at> felesatra.moe>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.1; xref-marker-ring-length user option doesn't have setter
Date: Wed, 26 Sep 2018 16:14:15 -0700

The xref-marker-ring-length user option doesn't have a setter, so
customizing it will not affect xref--marker-ring, except if/when user
customizations are loaded in a new Emacs process before xref is loaded.
Customizations after xref is loaded will have no effect.

In GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.22.24), modified by Debian
Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
System Description: Debian GNU/Linux rodete (upgraded from: Ubuntu 14.04 LTS)

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Allen Li <darkfeline <at> felesatra.moe>
Cc: 32849 <at> debbugs.gnu.org
Subject: Re: bug#32849: 26.1;
 xref-marker-ring-length user option doesn't have setter
Date: Thu, 27 Sep 2018 11:24:04 +0300

> From: Allen Li <darkfeline <at> felesatra.moe>
> Date: Wed, 26 Sep 2018 16:14:15 -0700
> 
> The xref-marker-ring-length user option doesn't have a setter, so
> customizing it will not affect xref--marker-ring, except if/when user
> customizations are loaded in a new Emacs process before xref is loaded.
> Customizations after xref is loaded will have no effect.

I think the setter should use ring-extend to enlarge the ring, and
some custom code using ring-remove to make the ring smaller.

Would you like to submit a patch along those lines?

Thanks.

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

From: Allen Li <darkfeline <at> felesatra.moe>
To: eliz <at> gnu.org
Cc: 32849 <at> debbugs.gnu.org
Subject: Re: bug#32849: 26.1;
 xref-marker-ring-length user option doesn't have setter
Date: Wed, 24 Oct 2018 21:30:51 -0600

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

Patches attached.

Of note:

I added a ring-resize function.

I wrote tests with names function-to-test/case-tested.  I'm not sure
if this violates style policies for tests.  It seems like existing
code tests multiple cases in a single test, but I find separate tests
easier to understand when they regress.  The / makes the test names
easier to read.

I based this off of 26 (as I think this is a minor bugfix/feature add).
On Thu, Sep 27, 2018 at 2:24 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Allen Li <darkfeline <at> felesatra.moe>
> > Date: Wed, 26 Sep 2018 16:14:15 -0700
> >
> > The xref-marker-ring-length user option doesn't have a setter, so
> > customizing it will not affect xref--marker-ring, except if/when user
> > customizations are loaded in a new Emacs process before xref is loaded.
> > Customizations after xref is loaded will have no effect.
>
> I think the setter should use ring-extend to enlarge the ring, and
> some custom code using ring-remove to make the ring smaller.
>
> Would you like to submit a patch along those lines?
>
> Thanks.

[0002-Add-setter-for-xref-marker-ring-length.patch (text/x-patch, attachment)]

[0001-Add-ring-resize-function.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Allen Li <darkfeline <at> felesatra.moe>
Cc: 32849 <at> debbugs.gnu.org
Subject: Re: bug#32849: 26.1;
 xref-marker-ring-length user option doesn't have setter
Date: Sat, 27 Oct 2018 12:57:47 +0300

> From: Allen Li <darkfeline <at> felesatra.moe>
> Date: Wed, 24 Oct 2018 21:30:51 -0600
> Cc: 32849 <at> debbugs.gnu.org
> 
> Patches attached.

Thanks.  Please see a few minor comments below.

> I wrote tests with names function-to-test/case-tested.  I'm not sure
> if this violates style policies for tests.  It seems like existing
> code tests multiple cases in a single test, but I find separate tests
> easier to understand when they regress.  The / makes the test names
> easier to read.

I don't see any problem with this, we don't have style policy that
disallows such style, AFAIK.

> I based this off of 26 (as I think this is a minor bugfix/feature add).

Hmm... I think we should install this on master, not on emacs-26, as
the problem is not serious enough.

> +** xref
> +
> ++++
> +*** Setter added for 'xref-marker-ring-length'.

This heading is not on the user level, it's on the implementation
level.  I'd suggest to use this instead:

  You can now change 'xref-marker-ring-length' after 'xref.el' is loaded.

> +if set before `xref.el` was loaded.

Please use quoting 'like this' in NEWS and in comments.

>  (defcustom xref-marker-ring-length 16
> -  "Length of the xref marker ring."
> -  :type 'integer)
> +  "Length of the xref marker ring.
> +If this variable is not set through Customize, you must call
> +‘xref-set-marker-ring-length’ for changes to take effect."

I guess you copy-pasted the last line from the *Help* buffer, or you
have electric-quotes turned on, because it uses the wrong quote
characters; in doc strings, please quote `like this' instead.

> +(defun xref-set-marker-ring-length (var val)
> +  "Set ‘xref-marker-ring-length’.
> +VAR is the symbol ‘xref-marker-ring-length’ and VAL is the new

Same here.

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

From: Noam Postavsky <npostavs <at> gmail.com>
To: Allen Li <darkfeline <at> felesatra.moe>
Cc: eliz <at> gnu.org, 32849 <at> debbugs.gnu.org
Subject: Re: bug#32849: 26.1;
 xref-marker-ring-length user option doesn't have setter
Date: Sun, 28 Oct 2018 14:32:49 -0400

Allen Li <darkfeline <at> felesatra.moe> writes:

> I wrote tests with names function-to-test/case-tested.  I'm not sure
> if this violates style policies for tests.  It seems like existing
> code tests multiple cases in a single test, but I find separate tests
> easier to understand when they regress.  The / makes the test names
> easier to read.
  
> +(ert-deftest ring-test-ring-resize/grow ()

Like Eli mentioned, there is no policy for this, but I'd say the
ring-test- prefix is redundant for test names, ring-resize/grow already
has the prefix ring- to avoid nameclashes with other tests (and test
namespace is separate from function namespace).

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

From: Allen Li <darkfeline <at> felesatra.moe>
To: eliz <at> gnu.org
Cc: 32849 <at> debbugs.gnu.org
Subject: Re: bug#32849: 26.1;
 xref-marker-ring-length user option doesn't have setter
Date: Thu, 8 Nov 2018 23:01:21 -0800

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

On Sat, Oct 27, 2018 at 2:57 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
> Thanks.  Please see a few minor comments below.

Thanks for your comments.  I have attached new patches based off master.

[0001-Add-ring-resize-function.patch (text/x-patch, attachment)]

[0002-Add-setter-for-xref-marker-ring-length.patch (text/x-patch, attachment)]

Message #27 received at 32849-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Allen Li <darkfeline <at> felesatra.moe>
Cc: 32849-done <at> debbugs.gnu.org
Subject: Re: bug#32849: 26.1;
 xref-marker-ring-length user option doesn't have setter
Date: Sat, 10 Nov 2018 11:47:29 +0200

> From: Allen Li <darkfeline <at> felesatra.moe>
> Date: Thu, 8 Nov 2018 23:01:21 -0800
> Cc: 32849 <at> debbugs.gnu.org
> 
> On Sat, Oct 27, 2018 at 2:57 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
> > Thanks.  Please see a few minor comments below.
> 
> Thanks for your comments.  I have attached new patches based off master.

Thanks, I pushed this to the master branch.

One nit:

> * doc/lispref/sequences.texi (Rings): Document new function
> * etc/NEWS: Document new function
> * lisp/emacs-lisp/ring.el (ring-resize): New function
> * test/lisp/emacs-lisp/ring-tests.el (ring-test-ring-resize): New tests

Please end each entry in the commit log with a period, as it's
supposed to be one or more complete sentences.

Previous Next

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