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 and swiper require emacs 24.4 #1629

Closed
JohnLunzer opened this issue Jun 15, 2018 · 10 comments

Comments

@JohnLunzer
Copy link

commented Jun 15, 2018

However, both counsel.el and swiper.el state that they require 24.1.

The culprit in both cases is the string-trim-right function called in only a few places. This is a very small function and it seems like a shame to give up a few minor versions of backward compatibility over a minor dependency.

Anyway... I'm not advocating, just providing you information. I've provided the function below which I copied from the emacs-mirror github page.

(defsubst string-trim-right (string &optional regexp)
  "Trim STRING of trailing string matching REGEXP.
REGEXP defaults to  \"[ \\t\\n\\r]+\"."
  (if (string-match (concat "\\(?:" (or regexp "[ \t\n\r]+") "\\)\\'") string)
      (replace-match "" t t string)
    string))
@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2018

See also #1579. I've already fixed the misuse of subr-x, just haven't created a coherent PR yet, FWIW.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2018

Note also that defsubst should generally be avoided, as its pitfalls usually outweigh its benefits in new code, and using replace-match is less efficient than using substring here (see my last patch here: https://lists.gnu.org/archive/html/emacs-devel/2018-06/msg00100.html).

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2018

This is a very small function and it seems like a shame to give up a few minor versions of backward compatibility over a minor dependency.

Agreed. subr-x is too experimental or not generally useful enough to use in packages which aim to support Emacs 24 or earlier.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 15, 2018

I vaguely remember trying to compile emacs-24.1 from sources on Ubuntu 16.04, and I could not manage to do it. I did manage to build emacs-24.3 just fine though. And I think emacs-24.3 is the version commonly provided by the emacs24 package on Debian derivatives. So I think the way to go is to bump the minimum version to 24.3, which is already the case for counsel.el. Please correct me if I got it wrong somewhere.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2018

I would be in favour of bumping the minimum required version, but I think subr-x can (and mostly should) be avoided regardless. Note that Debian Stable provides Emacs 24.5, OldStable provides Emacs 24.4, and string-trim-{left,right} were only added in Emacs 24.4.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 15, 2018

I think subr-x can (and mostly should) be avoided regardless

string-trim-right is a useful enough function. Why should we avoid using it?

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2018

string-trim-right is a useful enough function. Why should we avoid using it?

TL;DR it's more trouble than it's worth in terms of compatibility, program design, and performance.

The primary issue is compatibility: subr-x functions and their optional arguments were not all added in one go, and the Swiper project aims for compatibility with an Emacs version which lacks most string trimming functions in subr-x.

The secondary issue is that most code I've come across which uses subr-x string manipulation functions can, IME, be written in a simpler way which avoids them. It is rare, for example, for a well-designed program to want to eliminate carriage returns, tabs, newlines, and line feeds from a string all in one go, and, as I mentioned in a previous comment, the current implementations of string-trim-{left,right} are not as efficient as they should be. Don't forget that, in Elisp, complicated character manipulation should almost always be performed in buffers, not strings.

The main exceptions to my evaluation are truly trivial shorthands like string-join and string-empty-p, as well as the more convenient {if,when,and}-let macros, though these are so experimental that versions of them have alternated between being deprecated and un-deprecated across Emacs versions 25, 26, and 27. Although i use them wherever possible (let alone reasonable) in my own large configuration, IMO they seldom make a big difference in code size or legibility.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2018

Note also that, with any of the string manipulation functions, one saves at most 2 trivial lines of the type of code (regexp matching) that the Swiper project already makes extensive use of.

basil-conto added a commit to basil-conto/swiper that referenced this issue Jun 15, 2018
Fix subr-x usage
counsel.el (counsel-git-worktree-list, counsel-git-branch-list):
Let split-string do trimming, rather than doing it manually with
Emacs 24.4 string-trim-right.
doc/ivy-ox.el: Do not load subr-x during byte-code interpretation.
swiper.el (swiper-occur):
Remove subr-x runtime dependency and simplify.

Fixes abo-abo#1629

@abo-abo abo-abo closed this in 6f29394 Jun 15, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Jun 15, 2018

Note also that, with any of the string manipulation functions, one saves at most 2 trivial lines of the type of code (regexp matching) that the Swiper project already makes extensive use of.

Agreed. Although if we see those 2 trivial lines repeating all over the place, we might want to extract them into e.g. swiper--string-trim.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2018

if we see those 2 trivial lines repeating all over the place, we might want to extract them into e.g. swiper--string-trim

Oh, absolutely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.