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

counsel.el (counsel-compile-root-functions): Tidy #1968

Closed
wants to merge 1 commit into from

Conversation

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Mar 15, 2019

Revert introduction of partially applied functions (86c904f) in the definition of a variable, as they are inefficient, illegible in the *Help* buffer, and undocumented.

Revert introduction of partially applied functions[1] in the
definition of a variable, as they are inefficient, illegible in
the *Help* buffer, and undocumented.

[1]: counsel.el (counsel-compile-root-functions): Change priority
  2019-03-14 11:25:04 +0100
  abo-abo@86c904f
Copy link
Contributor

@stsquad stsquad left a comment

I feel as though I learnt something new about how to use apply-partially but I can see the benefit of clarity. I suppose if you wanted to be really terse you could make a macro that expanded out the helpers but that is probably overkill.

@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Mar 19, 2019

I feel as though I learnt something new about how to use apply-partially but I can see the benefit of clarity.

As much as I love functional programming styles, Elisp is not Haskell, and Emacs is not GHCi. The values and documentation of user-facing variables and functions should be human-readable. Users should not be punished because of programmers' personal preferences. Plain funcalls are already less efficient than desirable in Elisp, and apply-partially only makes things worse. Efficiency is obviously the least of our concerns in this particular example, but that combined with the loss of documentation sums up to an idiom worth avoiding, lest others pick up on the habit.

I suppose if you wanted to be really terse you could make a macro that expanded out the helpers but that is probably overkill.

Absolutely. Again, not really important in this tiny example, but macros often inevitably lead to compilation problems for users, so should be used as sparingly as is possible/reasonable.

A further issue with using apply-partially or similar is that it restricts customisability. Named functions are easier to advise and otherwise manipulate. There's simply no reason to spare a bit of typing in this trivial example.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 21, 2019

Thanks.

@basil-conto basil-conto deleted the blc/compile branch Mar 22, 2019
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Revert introduction of partially applied functions[1] in the
definition of a variable, as they are inefficient, illegible in
the *Help* buffer, and undocumented.

[1]: counsel.el (counsel-compile-root-functions): Change priority
  2019-03-14 11:25:04 +0100
  abo-abo@86c904f

Fixes abo-abo#1968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants