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

Various counsel-compile cleanups #1963

Closed
wants to merge 14 commits into from
Closed

Conversation

@basil-conto
Copy link
Collaborator

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

This is a follow-up to #1941, where some questions of mine are still pending an answer (I have marked the relevant sources with FIXMEs). Please review each mostly atomic commit in turn.

N.B.: These changes are 100% untested.

Cc: @stsquad, @Alexander-Shukaev

basil-conto added 14 commits Mar 12, 2019
(counsel--dominating-file): Rename from...
(counsel--find-project-root): ...this; change all callers.
Fix error when locate-dominating-file returns nil.
(counsel-git): Remove redundant file name expansion.
(counsel-locate-git-root, counsel-git-grep-function)
(counsel-git-grep, counsel-git-grep-proj-function)
(counsel-git-worktree-list, counsel-git-branch-list):
Capitalize "Git" as a proper noun where appropriate.
(counsel-compile--action): Rename from...
(counsel-compile--wrapper): ...this; change all callers.
Reconcile docstring with implementation.  Simplify logic.
(counsel-compile--update-history): Use add-to-history instead of
add-to-list and file-equal-p instead of string-equal.  Avoid one
level of unneeded string consing.
(counsel--get-compile-candidates): Follow Emacs docstring
conventions.  Avoid destructively modifying lists returned by
counsel-compile-local-builds elements.  Simplify logic.
(counsel-compile-get-filtered-history):
Simplify loop.  Do not mangle match data.  Fix variable names.
(counsel-compile-get-build-directories):
Remove one level of string consing.
Fix typo in docstring.  Replace the invalid rx form with
directory-files-no-dot-files-regexp.
(counsel-compile-get-make-invocation): Fix typo in docstring and
function name; change all callers.  Simplify logic.
Refill docstring.  Reduce amount of string consing.
(counsel-compile-history): Touch-up docstring.
(counsel-compile-root-function): Quote symbol as function.
(counsel-project-current): Use and instead of when.
(counsel-compile-local-builds): Refill docstring.
(counsel-compile-make-args): Clarify docstring.
(counsel-compile-make-pattern, counsel-compile-build-directories):
Fix :type tags and docstrings.
(counsel-compile-root-function): Remove, replacing with...
(counsel-compile-root-functions): ...this new variable.

(counsel-project-current): Remove, replacing with...
(counsel--compile-root): ...this new function.

(counsel--root-project, counsel--root-dir-locals)
(counsel--root-git): New functions.

(counsel-locate-git-root): Use counsel--root-git.

(counsel-compile-get-make-invocation)
(counsel-compile-get-build-directories)
(counsel-compile-get-filtered-history)
(counsel-compile--update-history): Use counsel--compile-root.

If you want to persist history between Emacs sessions you can as this
Copy link
Contributor

@stsquad stsquad Mar 13, 2019

Wouldn't it be worth the extra verbiage for those not familiar with what savehist-additional-variables does?

Copy link
Collaborator Author

@basil-conto basil-conto Mar 13, 2019

Users of savehist will know or have to learn how to use it anyway. It would absolutely be overkill and even a nuisance for every variable under the sun to mention that it can be persisted via savehist. Personally, I wouldn't mention it all, but there's no harm in keeping this one case. Besides, keeping this last sentence lets the docstring be auto-filled nicely.

(defcustom counsel-compile-make-pattern "\\(?:GNUM\\|[Mm]\\)akefile"
"Pattern for matching against makefiles."
:type 'regex)
(defcustom counsel-compile-make-pattern "\\`\\(?:GNUM\\|[Mm]\\)akefile\\'"
Copy link
Contributor

@stsquad stsquad Mar 13, 2019

huh, I missed that rx usage had been dropped. I guess no love for rx then...

Copy link
Collaborator Author

@basil-conto basil-conto Mar 13, 2019

Personally, I always use rx. I find its advantages far outweigh its disadvantages. Note, however, that #1941 made suboptimal and even incorrect usage of rx.

Copy link
Contributor

@stsquad stsquad left a comment

Aside from the minor document comments looks good.

Copy link
Contributor

@stsquad stsquad left a comment

Nice cleanup

@@ -5306,10 +5306,12 @@ to further refine the compile options in the directory specified by `blddir'."
(defun counsel-compile (&optional dir)
"Call `compile' completing with smart suggestions, optionally for DIR."
(interactive)
(add-hook 'compilation-start-hook 'counsel-compile--update-history)
;; No need to specify `:history' because of this hook.
(add-hook 'compilation-start-hook #'counsel-compile--update-history)
Copy link
Contributor

@stsquad stsquad Mar 13, 2019

@abo-abo did mention moving this to somewhere else but I'm not sure I see anywhere else being particularly better or worse.

Copy link
Collaborator Author

@basil-conto basil-conto Mar 13, 2019

The idea is that we modify a public hook without removing ourselves from it, and we don't even make use of the argument passed to each function in the abnormal hook. This isn't the end of the world, but it indicates that there's probably a cleaner way to call counsel-compile--update-history. I'm not yet familiar enough with this new logic to make any suggestions as to where this place could be, though.

'face 'font-lock-warning-face)
(propertize sd 'face 'dired-directory))
'srcdir srcdir
'blddir sd
Copy link
Contributor

@stsquad stsquad Mar 13, 2019

This changes behaviour - we want blddir to be the subdirectory so when we recurse we know where we are starting

Copy link
Contributor

@stsquad stsquad Mar 13, 2019

OK changing back the double propertize form while still avoiding the extra when gets things working again:

(defun counsel-compile-get-build-directories (&optional dir)
  "Return a list of potential build directories."
  (let* ((srcdir (or dir (counsel--compile-root)))
         (blddir (counsel--find-build-subdir srcdir)))
    (mapcar (lambda (s)
              (propertize
               (concat (propertize "Select build in "
                                   'face 'font-lock-warning-face)
                       (propertize s 'face 'dired-directory))
               'blddir s 'srcdir srcdir 'recursive t))
            (and blddir (counsel--get-build-subdirs blddir)))))

Copy link
Collaborator Author

@basil-conto basil-conto Mar 13, 2019

This changes behaviour - we want blddir to be the subdirectory so when we recurse we know where we are starting

Good catch, sorry about that thinko.

OK changing back the double propertize form while still avoiding the extra when gets things working again:

It's not the redundant propertize that fixes the problem; it's just the props I hoisted out of the loop weren't truly invariant. Too many propertize and concat calls can and should be avoided.

(dolist (item counsel-compile-history)
(let ((srcdir (get-text-property 0 'srcdir item))
(blddir (get-text-property 0 'blddir item)))
;; FIXME: File names are not regexps!
Copy link
Contributor

@stsquad stsquad Mar 13, 2019

Yes we should fix this. You could merge with c653867?

(mapcar #'car (sort (directory-files-and-attributes
blddir t directory-files-no-dot-files-regexp t)
(lambda (x y)
;; FIXME: Access time is NOT the 7th file attribute.
Copy link
Contributor

@stsquad stsquad Mar 13, 2019

Yep we should modify the docstring for modification time - as that is what will get updated when a new build is done.

Copy link
Contributor

@stsquad stsquad left a comment

Heh, here I was all this time let binding when I can just use the passed parameter!

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 13, 2019

Thanks.

@abo-abo abo-abo closed this Mar 13, 2019
abo-abo added a commit that referenced this issue Mar 13, 2019
(counsel-compile-root-function): Remove, replacing with...
(counsel-compile-root-functions): ...this new variable.

(counsel-project-current): Remove, replacing with...
(counsel--compile-root): ...this new function.

(counsel--root-project, counsel--root-dir-locals)
(counsel--root-git): New functions.

(counsel-locate-git-root): Use counsel--root-git.

(counsel-compile-get-make-invocation)
(counsel-compile-get-build-directories)
(counsel-compile-get-filtered-history)
(counsel-compile--update-history): Use counsel--compile-root.

Fixes #1963
@stsquad stsquad mentioned this pull request Mar 13, 2019
@basil-conto
Copy link
Collaborator Author

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

Heh, here I was all this time let binding when I can just use the passed parameter!

let-binding is more Lispy and functional, but it increases nesting and it can be hard to find meaningful names for every intermediate variable. It can also obfuscate how many bindings are actually needed. It's a subjective matter, though.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
(counsel-compile-root-function): Remove, replacing with...
(counsel-compile-root-functions): ...this new variable.

(counsel-project-current): Remove, replacing with...
(counsel--compile-root): ...this new function.

(counsel--root-project, counsel--root-dir-locals)
(counsel--root-git): New functions.

(counsel-locate-git-root): Use counsel--root-git.

(counsel-compile-get-make-invocation)
(counsel-compile-get-build-directories)
(counsel-compile-get-filtered-history)
(counsel-compile--update-history): Use counsel--compile-root.

Fixes abo-abo#1963
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