Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dash.el (->, ->>): Indent with 1 distinguished arg. #375

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

kiennq
Copy link
Contributor

@kiennq kiennq commented Mar 8, 2021

Per discussion in 9256290

@basil-conto basil-conto merged commit 250ec9f into magnars:master Mar 8, 2021
@basil-conto
Copy link
Collaborator

Thanks!

@basil-conto basil-conto added the enhancement Suggestion to improve or extend existing behavior label Mar 8, 2021
@basil-conto basil-conto added this to the 2.19.0 milestone Mar 8, 2021
@yyoncho
Copy link
Contributor

yyoncho commented Apr 12, 2021

FWIW this indentation change makes the indentation of -> and ->> different from the indentation they have in clojure which IMHO introduces redundant cognitive load.

(cc @ericdallo)

@ericdallo
Copy link

ericdallo commented Apr 12, 2021

Yes, I thought this was an issue with my emacs config, but I just realized it's because of this change, I totally agree that the indentation should be like clojure

@basil-conto
Copy link
Collaborator

I don't feel too strongly about this, but IMO it makes more sense to follow Elisp conventions and idioms when writing Elisp (this applies to any language/environment, not just Elisp). For example, Elisp indents if differently to some other Lisps.

@magnars @Fuco1 Do you favour reverting to Clojure indentation?

Thanks.

@magnars
Copy link
Owner

magnars commented Apr 13, 2021 via email

@basil-conto
Copy link
Collaborator

basil-conto commented Apr 13, 2021

Since I code clojure all day, I personally prefer the Clojure indentation. That doesn't mean it's necessarily right for this package, tho.

Well, you know that I stand in the latter camp, not only because I don't like the idea of imposing another language's style on users of Dash who are writing Elisp; and there's no shortage of examples where Elisp is subtly different from other Lisps - that's par for the course. I'm sure that even within Dash we're not always consistent in which style we follow - that's also to be expected.

But luckily what I think doesn't matter that much, and I defer to your preferred package-wide style. Whatever we decide, let's just be consistent ;).

However, since there is now thread-first and thread-last macros in subr-x.el, maybe our threading macros can have a Clojure flavor?

Given the vast breadth of Dash's API, I'm not sure the presence of just thread-first and thread-last should make any difference for us. If anything, I feel the opposite: since these macros exist upstream, we ought to be consistent with their indentation for the benefit of those using them (don't be different for the sake of being different, etc.). But like I said, I don't feel too strongly either way.

@yyoncho
Copy link
Contributor

yyoncho commented Apr 13, 2021

I don't like the idea of imposing another language's style on users of Dash who are writing Elisp

The threading macros themselves are imposing another language style into elisp. And not only these macros - the whole package is breaking much more important elisp conventions(like package prefix and functions ending with ?) in favor of being practical and convenient.

@basil-conto
Copy link
Collaborator

basil-conto commented Apr 13, 2021

The threading macros themselves are imposing another language style into elisp.

Sorry, I don't follow this logic - macros don't constitute a programming/editing language/environment. We're talking about the syntax of Elisp, not the semantics of Elisp definitions.

And not only these macros - the whole package is breaking much more important elisp conventions(like package prefix and functions ending with ?)

See #213 and similar discussions re: ? vs -p.

in favor of being practical and convenient.

I don't see how Elisp-style indentation is less practical or convenient when Emacs does it for you. Besides, the Elisp-style indentation is less indented, so in my book that's actually more "practical and convenient".

We can agree to disagree ;).

@yyoncho
Copy link
Contributor

yyoncho commented Apr 13, 2021

It is less practical for multiple reasons.

  • It makes code less readable since -> and ->> are harder to distinguish. It makes a huge difference when your organization is using -> or ->> 250+ times.
  • As I already mentioned it introduces redundant cognitive load - all lisps having these arrows use this formatting. There are zero benefits from following that convention - if someone knows about it - most likely they won't use dash at all.
  • Even your "practical and convenient" point about less indentation is not in your favour... This is before:
(->> 
 5 
 (+ 20)
 (/ 25)
 -
 (+ 40))

@basil-conto
Copy link
Collaborator

basil-conto commented Apr 13, 2021

It is less practical for multiple reasons.

I said "in my book" because what counts as practical, readable, cognitively cumbersome, etc. is clearly a personal and subjective issue.

Whether undistinguished macro arguments are indented according to Clojure or Elisp conventions is less subjective, but still a stylistic choice. BTW, this being Emacs, the indentation can of course be customised per-user, per-project, etc.

I've already given my context for why I personally follow each language's conventions when writing in that language. The decision for what to do in Dash isn't mine to make anyway.

Even your "practical and convenient" point about less indentation is not in your favour... This is before:

According to my book, that indentation is wrong, so any argument following from that is moot ;).

@yyoncho
Copy link
Contributor

yyoncho commented Apr 13, 2021

I personally follow common sense and the solutions that will be most useful for the actual users. I also try to avoid inflicting pain and redundant work for stuff that I don't feel strongly about.

@basil-conto
Copy link
Collaborator

@magnars Your comments sounded like they were in favour of following Clojure indentation project-wide. Let me know if this is indeed the case and I'll make the necessary changes.

@magnars
Copy link
Owner

magnars commented May 5, 2021

I am not involved enough to impose my will in that way. I can certainly see the value of sticking to the Emacs lisp conventions as well. In the end dash is a collection of tools for use with Elisp, so let's go for that.

@basil-conto
Copy link
Collaborator

I am not involved enough to impose my will in that way.

I really respect that, but as the package's author and with feet in both the Elisp and Clojure pools, your will carries unique weight :). But I also understand that bikesheds aren't the best hill to die on ;).

In the end dash is a collection of tools for use with Elisp, so let's go for that.

Agreed.

@basil-conto
Copy link
Collaborator

We already have dash-fontify-mode for font-locking some Dash-isms in Elisp buffers.

Would it make sense to add something like a dash-clojure-indent-mode for picking between more Elisp-like and Clojure-like indentation? (Or conversely a dash-elisp-indent-mode, if Clojure-like indentation becomes the default in this scenario.)

This would allow projects to specify e.g.

((emacs-lisp-mode (mode . dash-clojure-indent)))

or

((emacs-lisp-mode (mode . dash-elisp-indent)))

in their dir-locals-file for easily picking one over the other.

@basil-conto basil-conto added this to In Progress in Release 2.19.0 Jun 3, 2021
@basil-conto basil-conto moved this from In Progress to To Do in Release 2.19.0 Jun 3, 2021
@magnars
Copy link
Owner

magnars commented Jun 4, 2021

That sounds pretty good to me! With elisp being the default, then.

@basil-conto basil-conto moved this from To Do to Blocking in Release 2.19.0 Jun 29, 2021
@basil-conto basil-conto mentioned this pull request Jun 29, 2021
@basil-conto basil-conto moved this from Blocking to To Do in Release 2.19.0 Jun 29, 2021
@alphapapa
Copy link

IMHO "we" (not being a maintainer myself) should avoid unnecessary indentation changes; or, at least, space them out years apart. I just had a spate of dash-related indentation changes in one of my projects, and that pattern will repeat for most of my others the next time I work on them. Not only is it annoying to have to examine the diffs and commit these indentation changes manually, but they cause new linting warnings on CI until I update the package; and these warnings were not caused by any changes in my packages, they just start happening when dash is updated on MELPA. Dash is supposed to save Elisp developers work, not create more for them. ;)

@basil-conto
Copy link
Collaborator

basil-conto commented Jun 30, 2021

Sorry, I thought of the indentation changes as fixes/cleanups, but I can't in good conscience controversially force them on so many Elisp authors any more. Backward compatibility wins again!

@magnars I'm guessing you don't have a problem with reconsidering this decision. If so, for the upcoming 2.19.0 release in #382 I'll try to revert any breaking indentation changes, and add Elisp-style indentation as an opt-in feature if I have time.

Sorry again for the double hassle and thanks for bearing with my stubbornness.

@magnars
Copy link
Owner

magnars commented Jun 30, 2021

That's a good point. Let's keep it as-is, then.

@alphapapa
Copy link

@basil-conto Thank you for being so considerate of downstream users! :) Not all Elisp packages are developed with such care, and pleas like mine often fall on deaf ears elsewhere.

@basil-conto
Copy link
Collaborator

Ugh, the status quo (even before this PR) is a bit all over the place in terms of indentation style, see e.g. -doto, --doto, #319, #321, etc.

So my conclusion is basically to revert this PR that I'm partially responsible for and call it a day ;).

basil-conto added a commit that referenced this pull request Jul 4, 2021
This reverts commit 9256290
"* dash.el (-->): Indent with 1 distinguished arg."
of 2021-01-10.

This reverts commit 911ef09
"* dash.el (->, ->>): Indent with 1 distinguished arg."
of 2021-03-08.

The Clojure-inspired threading macros -> and ->> are quite popular
and were indented until recently with no distinguished arguments as
per Clojure conventions.  In Elisp practice this is not a problem
since the macros have short names.  For the converse argument
against threading macros such as -some-> that have longer names, see
the discussion in #319 and #321.

This commit reverts relatively recent breaking and controversial
indentation changes that tried to make ->, ->>, and --> more
consistent with the rest of Dash, including -some->, -doto,
etc. which are all indented with a distinguished argument despite
being inspired by Clojure.  For the relevant discussion, see #375.

* dash.el (->, ->>, -->): Indent without a distinguished argument.
(-as->, -some->, -some->>, -fixfn): Reindent.
* NEWS.md (2.19.0): Announce indentation change to -->.
@basil-conto
Copy link
Collaborator

So my conclusion is basically to revert this PR that I'm partially responsible for and call it a day ;).

I've now done so in commit 3bd52a4. Please let me know if any outstanding issues remain.

@basil-conto basil-conto moved this from To Do to Done in Release 2.19.0 Jul 4, 2021
alphapapa added a commit to alphapapa/org-ql that referenced this pull request Mar 9, 2023
See, e.g. <magnars/dash.el#375>.

Ultimately I'd prefer to indent with one distinguished argument, but
the noise it causes downstream is likely not worth it.  (And,
unfortunately, the indentation of the built-in thread-first and
thread-last forms has changed to be like this as well, which is even
worse, causing 10-11 characters of extra indentation in those forms'
bodies!)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement Suggestion to improve or extend existing behavior
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants