Open
Conversation
basil-conto
reviewed
Sep 13, 2024
basil-conto
left a comment
basil-conto
left a comment
There was a problem hiding this comment.
Thanks, just a couple of comments in passing from me.
| lines)) | ||
| (min-indent (-min (--map (length it) indents)))) | ||
| (let* ((lines (split-string string "\n")) | ||
| (min-indent most-positive-fixnum) (indent 0)) |
There was a problem hiding this comment.
-
Elisp has bignums now so I would find it more elegant to do away with the arbitrary fixnum limit, e.g. by continuing to use Dash helpers (min-by, reduce), or by using
nilor similar as an initial value. -
The
indentvariable can be moved into the loop to avoid the need forsetq(which should make it slightly more efficient too).
| (let* ((lines (split-string string "\n")) | ||
| (min-indent most-positive-fixnum) (indent 0)) | ||
| (dolist (line lines) | ||
| (setq indent (or (string-match-p "[^[:blank:]]" line) 0)) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi Wilfred, is this still interesting to you?
Wilfred/helpful#1
#1
Dash is included in Elpa, so it is really only s.el left :-).
Regardless of including into Elpa or Emacs, I would like to use your package in a project of mine, at least for a while. For my own preference, I have removed dependency on s.el. If you check the patch it wasn't really much to do, it is barely like 10 lines of code. I did have to change the algorithm in you 'elisp-refs--unindent-rigidly', since I couldn't come up with a pretty replacement for 's-match'. The new one should be a slight improvement since it does not use intermediate list to find the minimum but does it in-place, but it is really nothing noticeable, the search is anyway dominated by I/O and string comparisons, not by this function.
The compatibility should go back to Emacs 24.4. It is 'string-join' that it is the "newest" of the bunch, otherwise the patch would be compatible with 24.1.
If you are considering to contribute this to Emacs core or Elpa, I personally have signed the FSF copyrights; but this would probably not even count as a "significant" contribution.
Best regards, thanks for the Helpful! I have been using it a lot, and really like it.