Re: 2a73673 Change how thread-first/thread-last indent the first argument

Re: 2a73673 Change how thread-first/thread-last indent the first argument

[Adam Porter]

Hi Lars, et al,

In this commit, you changed the thread-last macro's indentation from
(indent 1) to (indent 0).  Could this be reverted, please?  This change
has two drawbacks:

1.  The threaded form is no longer indented differently from the other
forms, so it no longer stands out.  In some code, this makes it harder
to read.  For example, in deffy.el (an example application in the
taxy.el repo), this form is now much harder to read:

(thread-last
  (make-fn
   :name "Deffy"
   :description
   (format "Definitions in %s:"
           (if files
               (string-join (mapcar #'file-relative-name files) ", ")
             (file-name-nondirectory
              (directory-file-name (project-root project)))))
   :take (taxy-make-take-function keys deffy-keys))
  (taxy-fill forms)
  (taxy-sort* #'string< #'taxy-name)
  (taxy-sort #'string< #'def-name))

Whereas before, it looked like this:

(thread-last
    (make-fn
     :name "Deffy"
     :description
     (format "Definitions in %s:"
             (if files
                 (string-join (mapcar #'file-relative-name files) ", ")
               (file-name-nondirectory
                (directory-file-name (project-root project)))))
     :take (taxy-make-take-function keys deffy-keys))
  (taxy-fill forms)
  (taxy-sort* #'string< #'taxy-name)
  (taxy-sort #'string< #'def-name))

Without the extra indentation for the first form, it's hard to see where
the threaded form ends and the threaded-into forms begin.

2.  The change in indentation results in "indentation churn" in every
downstream package that uses thread-last or thread-first.  For example,
I use aggressive-indent-mode when editing Elisp packages, to ensure that
they're always indented properly, and now all of these forms get
reindented, and it makes extra diffs to scrutinize when committing other
changes in Magit.

Thanks for your consideration.

Adam

Re: 2a73673 Change how thread-first/thread-last indent the first argument

[Lars Ingebrigtsen]

Adam Porter <adam@alphapapa.net> writes:

> In this commit, you changed the thread-last macro's indentation from
> (indent 1) to (indent 0).

The change made the macro indent the way it was documented to indent (in
the doc string):

  "Thread FORMS elements as the first argument of their successor.
Example:
    (thread-first
      5
      (+ 20)
      (/ 25)
      -
      (+ 40))

> Could this be reverted, please?  This change
> has two drawbacks:
>
> 1.  The threaded form is no longer indented differently from the other
> forms, so it no longer stands out.

I don't really have an opinion here -- I can revert the change if that's
what people that use the macro wants.  It just looked really wrong to me
to indent the first element specially, but I don't use these macros in
Emacs.  I don't think ->> indents the first element specially in
Clojure, for instance?  (At least I don't remember them doing so.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: 2a73673 Change how thread-first/thread-last indent the first argument

[Adam Porter]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Adam Porter <adam@alphapapa.net> writes:
>
>> In this commit, you changed the thread-last macro's indentation from
>> (indent 1) to (indent 0).
>
> The change made the macro indent the way it was documented to indent (in
> the doc string):
>
>   "Thread FORMS elements as the first argument of their successor.
> Example:
>     (thread-first
>       5
>       (+ 20)
>       (/ 25)
>       -
>       (+ 40))

It would only be a two-character, whitespace-only change to "fix" the
docstring.  ;)

>> Could this be reverted, please?  This change
>> has two drawbacks:
>>
>> 1.  The threaded form is no longer indented differently from the other
>> forms, so it no longer stands out.
>
> I don't really have an opinion here -- I can revert the change if that's
> what people that use the macro wants.  It just looked really wrong to me
> to indent the first element specially, but I don't use these macros in
> Emacs.  I don't think ->> indents the first element specially in
> Clojure, for instance?  (At least I don't remember them doing so.)

You're right about Clojure.  And, in fact, Dash.el does follow Clojure
there in its implementation of `->>', et al.  However, earlier this
year, Basil adjusted its indentation to be in line with how
`thread-last' used to indent, with the first form indented more:

https://github.com/magnars/dash.el/pull/375

And perhaps ironically, it was me who convinced him to revert that
change and restore the Clojure-like indentation.  My reasoning was the
same as my second reason posted here: it results in lots of whitespace
churn downstream.

At the same time, I agree with Basil that we need not follow Clojure's
conventions, and using first-form indentation is more useful.

Another issue is that, since older Emacs versions persist in the wild
for years, this will result in code changing indentation between Emacs
versions: if someone submits a patch based on Emacs <28, it will have
one indentation, and if the maintainer uses Emacs 28, it will have
another indentation.

Finally, changing the indentation will break indentation linters.  For
example, in https://github.com/alphapapa/makem.sh I use the lint-indent
rule to ensure that code is correctly indented, and I test some of my
packages on multiple Emacs versions in CI.  This change will cause
linting to always fail on one Emacs version or another, so it won't be
possible to have a clean linting anymore.

So if this were reverted, I'd be grateful.

Re: 2a73673 Change how thread-first/thread-last indent the first argument

[Basil L. Contovounesios]

Adam Porter [2021-10-05 03:41 -0500] wrote:

> You're right about Clojure.  And, in fact, Dash.el does follow Clojure
> there in its implementation of `->>', et al.  However, earlier this
> year, Basil adjusted its indentation to be in line with how
> `thread-last' used to indent, with the first form indented more:
>
> https://github.com/magnars/dash.el/pull/375
>
> And perhaps ironically, it was me who convinced him to revert that
> change and restore the Clojure-like indentation.  My reasoning was the
> same as my second reason posted here: it results in lots of whitespace
> churn downstream.
>
> At the same time, I agree with Basil that we need not follow Clojure's
> conventions, and using first-form indentation is more useful.

FWIW, the discussion you link is only the latest instance of Dash going
back and forth on this issue:

https://emacs.stackexchange.com/a/66623/15748

-- 
Basil

Re: 2a73673 Change how thread-first/thread-last indent the first argument

[Lars Ingebrigtsen]

Adam Porter <adam@alphapapa.net> writes:

> Another issue is that, since older Emacs versions persist in the wild
> for years, this will result in code changing indentation between Emacs
> versions: if someone submits a patch based on Emacs <28, it will have
> one indentation, and if the maintainer uses Emacs 28, it will have
> another indentation.

That's true of any indentation change, but we do make those, anyway.  In
Emacs 28 we changed how

'( foo
   bar)

is indented, and in Emacs 29 we've changed how cl-flet is indented.

(The first change probably didn't entail any churn because nobody wrote
something like that before, but the cl-flet one does introduce churn.)

So the question is whether the old indentation was a bug or not, and it
looks like a bug to me.  That is, the only documentation of the form we
have indents the way it does now (after the change).  On the other hand,
the documentation could be incorrect.

I don't really have much of an opinion here -- but the old indentation
looks like a bug to me.  If the thread-first/last community disagrees
with this, I'm open to reverting the patch.  Does anybody else have an
opinion here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: 2a73673 Change how thread-first/thread-last indent the first argument

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I don't really have much of an opinion here -- but the old indentation
> looks like a bug to me.  If the thread-first/last community disagrees
> with this, I'm open to reverting the patch.  Does anybody else have an
> opinion here?

I don't have a strong opinion, but I'm personally not too concerned
about some churn given that "git blame" has an option to ignore
whitespace changes.  (Magit uses that flag by default, and if I'm not
mistaken VC does too.)

As for aggressive-indent, it does sound like a somewhat unusual
use-case.  I only re-indent code specifically when it makes sense.
When looking over my diff before committing, I usually double-check to
see that I'm not accidentally doing some random re-indenting unless
I'm also changing those lines, or otherwise have some specific reason
to do it (for example, if it substantially improves readability).

Just my two cents.

Re: 2a73673 Change how thread-first/thread-last indent the first argument

[Adam Porter]

On Wed, Oct 6, 2021 at 5:27 AM Stefan Kangas <stefan@marxist.se> wrote:
>
> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
> > I don't really have much of an opinion here -- but the old indentation
> > looks like a bug to me.  If the thread-first/last community disagrees
> > with this, I'm open to reverting the patch.  Does anybody else have an
> > opinion here?
>
> I don't have a strong opinion, but I'm personally not too concerned
> about some churn given that "git blame" has an option to ignore
> whitespace changes.  (Magit uses that flag by default, and if I'm not
> mistaken VC does too.)

FWIW, the burden for me comes when I review changes in Magit before
committing: even whitespace-change-only hunks have to be examined and
discarded.

> As for aggressive-indent, it does sound like a somewhat unusual
> use-case.  I only re-indent code specifically when it makes sense.
> When looking over my diff before committing, I usually double-check to
> see that I'm not accidentally doing some random re-indenting unless
> I'm also changing those lines, or otherwise have some specific reason
> to do it (for example, if it substantially improves readability).

When editing other projects, I may disable aggressive-indent-mode if
it's causing too much churn.  Regardless, I review changes in Magit,
and if there are indentation fixes to be made, I commit them
separately from the changes I'm making (so if the changes need to be
reverted later, the whitespace fixes remain).

I generally recommend using aggressive-indent-mode for Elisp projects,
because IME (limited compared to that of others present here, of
course), patches submitted by contributors who don't use it very often
have incorrectly indented forms (sometimes subtly so, only off by a
character, which can go unnoticed until much later, or which can be
obscured depending on whether tabs have snuck in).  And when I'm
coding, it saves me from having to think about whether code is
indented correctly, having to manually reindent forms after minor
changes, etc.  It just DTRT so the code is always indented correctly,
which is a relief.

Re: 2a73673 Change how thread-first/thread-last indent the first argument

[Adam Porter]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> That's true of any indentation change, but we do make those, anyway.  In
> Emacs 28 we changed how
>
> '( foo
>    bar)
>
> is indented

I would argue that that's introducing a new feature rather than changing
how an existing form is indented, because such a form with a space
before the first element didn't indent differently than one without a
space, so there was no reason to write a form that way before this
change.

> and in Emacs 29 we've changed how cl-flet is indented.

> (The first change probably didn't entail any churn because nobody wrote
> something like that before, but the cl-flet one does introduce churn.)

Yes, but that's fixing a longstanding bug, a clear incompatibility with
CL, and the unfixed cl-flet indentation is purely a drawback (causing
too-long lines where they'd otherwise be within fill-column).

> So the question is whether the old indentation was a bug or not, and it
> looks like a bug to me.  That is, the only documentation of the form we
> have indents the way it does now (after the change).  On the other hand,
> the documentation could be incorrect.

Looking at vc-region-history for thread-last, it's only been touched 3
times: most recently in this indentation change, then in 2014 when Paul
Eggert fixed a typo in the docstring, and firstly in the same year when
Fabián Ezequiel Gallina introduced the macro.  I would argue that Fabián
wrote "(declare (indent 1))" intentionally, and the fact that the "5" in
the docstring example was not indented by two more spaces was an
oversight, probably explained by having manually indented the code in
the docstring.

So I would argue that the bug was in the example in the docstring, not
in the code.  And given that offsetting the first form is more useful,
and that it's in line with other macros whose first form is special
(e.g. `prog1'), the indentation for `thread-first' and `thread-last'
should not be changed to match the erroneous docstring.  It would be
preferable to just fix the docstring.