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 compile fixes #1966

Closed
wants to merge 4 commits into from
Closed

Conversation

@stsquad
Copy link
Contributor

@stsquad stsquad commented Mar 13, 2019

Fixes a minor bug introduced with #1963 plus a bit more commentary.

counsel.el Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
counsel.el Show resolved Hide resolved
stsquad added 3 commits Mar 13, 2019
Commit 1439e8d broke subdir recursion by only passing the directory
containing the subdirs rather than the final subdir. Fix this by
partially reverting the change while keeping the new cleaner
structure.
Add some commentary and use file-in-directory-p instead of treating
paths and regexes.
@stsquad stsquad force-pushed the counsel-compile-fixes branch from 4187746 to 3120f1c Mar 13, 2019
@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 13, 2019

I've just updated the branch.

@basil-conto
Copy link
Collaborator

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

Thanks, LGTM. Can you please also add the following change, as discussed in #1941 (review) ?

diff --git a/counsel.el b/counsel.el
index f8e6e8a..0909244 100644
--- a/counsel.el
+++ b/counsel.el
@@ -5260,10 +5260,7 @@ counsel--get-compile-candidates
   "Return the list of compile commands.
 This is determined by `counsel-compile-local-builds', which see."
   (let (cands)
-    ;; FIXME: Shouldn't `counsel-compile-local-builds' always be a list?
-    (dolist (cmds (if (listp counsel-compile-local-builds)
-                      counsel-compile-local-builds
-                    (list counsel-compile-local-builds)))
+    (dolist (cmds counsel-compile-local-builds)
       (when (functionp cmds)
         (setq cmds (funcall cmds dir)))
       (when cmds

Remove the clumsy `listp' check which is not needed as
counsel-compile-local-builds is documented to always be a list.
@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 13, 2019

Thanks, LGTM. Can you please also add the following change, as discussed in #1941 (review) ?

Yes, done. @abo-abo I think this is all ready now.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 14, 2019

counsel-locate-git-root and counsel--root-git are inconsistent names, can we change to counsel--git-root?

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 14, 2019

(defvar counsel-compile-root-functions
  '(counsel--root-project
    counsel--root-dir-locals
    counsel--root-git)

I think root-git is more important than dir-locals. Plus, we could also check for files like Makefile.in or Makefile, e.g. root-makefile. Furthermore, since Emacs itself is build with GNU Autotools, I think we should also respect that and have root-configure (or root-gnu-autotools) to search for configure.ac or configure. The overall priority should be:

(defvar counsel-compile-root-functions
  '(counsel--root-project
    counsel--root-configure
    counsel--root-git
    counsel--root-makefile
    counsel--root-dir-locals)

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 14, 2019

(defvar counsel-compile-root-functions
(defvar counsel-compile-local-builds

Those are defcustom in fact or?

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 14, 2019

(defcustom counsel-compile-make-pattern "\\`\\(?:GNUM\\|[Mm]\\)akefile\\'"
  "Regexp for matching the names of Makefiles."
  :type 'regexp)

Please, refer to Makefile Names, there is no GNUMakefile but rather GNUmakefile.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 14, 2019

counsel--get-make-targets should be counsel-compile--get-make-targets as it is part of counsel-compile framework due to coupling with string properties.

@abo-abo abo-abo closed this in 25336fd Mar 14, 2019
abo-abo added a commit that referenced this issue Mar 14, 2019
abo-abo added a commit that referenced this issue Mar 14, 2019
* counsel.el (counsel--project-current): Rename.
(counsel--root-dir-locals): Remove. Avoid functions that are just
`apply-partially' if they are used only in one place.

Re #1966
@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 14, 2019

I think the documentation should be more clear and reflect what the return values of functions really are. So counsel--get-make-targets in fact returns a list of commands (not targets) per target. I would update the documentation accordingly and rename the function to counsel-compile--get-make-commands. Same goes for counsel-compile-get-make-invocation, it should rather be counsel-compile-get-make-commands.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 14, 2019

Thanks, all.

@Alexander-Shukaev I've addressed most of your comments, please PR for the missing ones.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 14, 2019

Happy to do it. Is it OK if I don't have papers signed with GNU? Would you be able to commit on your name my PR?

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 14, 2019

@Alexander-Shukaev I can't do that, unfortunately. Are you not able to sign the papers with the FSF?

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 14, 2019

Well, this will take time then. Just wondering why is it not possible to just pull the branch from PR, squash it and amend the author as yourself and then push to master? Is it explicitly not allowed by those papers?

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 14, 2019

Well, this will take time then.

No problem.

... amend the author as yourself and then push to master? Is it explicitly not allowed by those papers?

I don't want to do that. I want to give credit to the person that made the change.

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 14, 2019

I just went through Copyright Papers, and AFAIU, somebody with access to fencepost.gnu.org/gd/gnuorg/Copyright/request-assign.changes has to send me this file and I should then fill it in and send it elsewhere (where?) to get the actual papers in return?

@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 14, 2019

I just went through Copyright Papers, and AFAIU, somebody with access to fencepost.gnu.org/gd/gnuorg/Copyright/request-assign.changes has to send me this file and I should then fill it in and send it elsewhere (where?) to get the actual papers in return?

Hmm I just emailed the application to assign@gnu.org with my full name and address then printed and signed the pdf I was sent and scanned it and sent it back. I got another copy once it was signed by the FSF lawyer.

@basil-conto
Copy link
Collaborator

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

Hmm I just emailed the application to assign@gnu.org with my full name and address then printed and signed the pdf I was sent and scanned it and sent it back. I got another copy once it was signed by the FSF lawyer.

I did the same. The whole exchange took about a couple of weeks. As documented under (maintain) Copyright Papers, you can find the forms here: http://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright

@basil-conto
Copy link
Collaborator

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

counsel-locate-git-root and counsel--root-git are inconsistent names, can we change to counsel--git-root?

The latter is meant to be consistent with other counsel--root-* functions, but I ultimately don't mind what they're called.

counsel--get-make-targets should be counsel-compile--get-make-targets as it is part of counsel-compile framework due to coupling with string properties.

True, but long names are less legible and a pain to work with, and if the purpose of the function isn't coupled to compile then there's no need to couple its name as well. The naming of internal functions doesn't have to be as sanitised as that of public ones.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Remove the clumsy `listp' check which is not needed as
counsel-compile-local-builds is documented to always be a list.

Fixes abo-abo#1966
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
* counsel.el (counsel--project-current): Rename.
(counsel--root-dir-locals): Remove. Avoid functions that are just
`apply-partially' if they are used only in one place.

Re abo-abo#1966
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
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

4 participants