GNU bug report logs - #20161
Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el

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 20161 in the body.
You can then email your comments to 20161 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: Jackson Hamilton <jackson <at> jacksonrayhamilton.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el
Date: Sat, 21 Mar 2015 13:20:09 -0700

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

No one on emacs-devel seemed to notice, so I'm pushing this on to the bug
mailing list. If someone could please review this I'd appreciate it.

---------- Forwarded message ----------
From: Jackson Hamilton <jackson <at> jacksonrayhamilton.com>
Date: Sat, Mar 7, 2015 at 4:49 PM
Subject: Fwd: Requesting review for change to lisp/textmodes/sgml-mode.el
To: emacs-devel <emacs-devel <at> gnu.org>


Hey guys, still hoping to get this reviewed. I wouldn't want to merge
something in that wasn't given the "A-OK."

---------- Forwarded message ----------
From: Jackson Hamilton <jackson <at> jacksonrayhamilton.com>
Date: Wed, Feb 25, 2015 at 2:35 AM
Subject: Requesting review for change to lisp/textmodes/sgml-mode.el
To: emacs-devel <emacs-devel <at> gnu.org>


Hello comrades,

I made an adjustment (fix?) to the way SGML attributes are indented.

Previously, if one wrote a form like the following:

<element attribute="value">

He could break the attribute onto a new line and it would be indented like
so:

<element
   attribute="value">

But sgml-basic-offset defaults to 2, not 3, so it doesn't make much sense
that
the attribute is indented by 3 spaces.

And if I (setq sgml-basic-offset 4), now my attributes are indented by 5
spaces. Personally I do not expect this behavior, I expect the indentation
to
match.

Perhaps it could be argued that the extra space helps to improve
readability;
maybe so, but it still seems to contradict the offset value. In teams where
many
people use editors that insert multiples of N spaces or tabs, this +1
indentation strategy feels rather alienating. I think it would be better to
stick to a multiple of the specified offset when an attribute is sitting on
its
own line.

Hence the attached patch to remove the +1 indentation behavior.

Thanks for reviewing,
Jackson

[Message part 2 (text/html, inline)]

[0001-lisp-textmodes-sgml-mode.el-sgml-calculate-indent-Fi.patch (text/x-patch, attachment)]

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

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Jackson Hamilton <jackson <at> jacksonrayhamilton.com>
Cc: 20161 <at> debbugs.gnu.org
Subject: Re: bug#20161: Fwd: Requesting review for change to
 lisp/textmodes/sgml-mode.el
Date: Sun, 22 Mar 2015 10:03:59 -0400

> He could break the attribute onto a new line and it would be indented like
> so:

> <element
>    attribute="value">

> But sgml-basic-offset defaults to 2, not 3, so it doesn't make much sense
> that
> the attribute is indented by 3 spaces.

As is clear from the code you're changing the "1+" is quite deliberate,
which shows some people prefer it that way.

So as discussed, this indentation step should have its own custom var.
You can make it default to the behavior you prefer if you want.

Other than that, the patch looks OK.


        Stefan

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

From: Jackson Hamilton <jackson <at> jacksonrayhamilton.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 20161 <at> debbugs.gnu.org
Subject: Re: bug#20161: Fwd: Requesting review for change to
 lisp/textmodes/sgml-mode.el
Date: Sun, 22 Mar 2015 08:39:22 -0700

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

Okay, created a defcustom for it. Patch is attached.

On Sun, Mar 22, 2015 at 7:03 AM, Stefan Monnier <monnier <at> iro.umontreal.ca>
wrote:

> > He could break the attribute onto a new line and it would be indented
> like
> > so:
>
> > <element
> >    attribute="value">
>
> > But sgml-basic-offset defaults to 2, not 3, so it doesn't make much sense
> > that
> > the attribute is indented by 3 spaces.
>
> As is clear from the code you're changing the "1+" is quite deliberate,
> which shows some people prefer it that way.
>
> So as discussed, this indentation step should have its own custom var.
> You can make it default to the behavior you prefer if you want.
>
> Other than that, the patch looks OK.
>
>
>         Stefan
>

[Message part 2 (text/html, inline)]

[0001-Have-sgml-attribute-offset-control-SGML-attribute-in.patch (text/x-patch, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jackson Hamilton <jackson <at> jacksonrayhamilton.com>
Cc: 20161 <at> debbugs.gnu.org
Subject: Re: bug#20161: Fwd: Requesting review for change to
 lisp/textmodes/sgml-mode.el
Date: Sun, 22 Mar 2015 22:02:39 -0400

> Okay, created a defcustom for it. Patch is attached.

Fine by me, thank you,


        Stefan

Previous Next

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