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

Feature request: add integration with project.el #1282

Closed
ambihelical opened this issue Sep 18, 2018 · 24 comments
Closed

Feature request: add integration with project.el #1282

ambihelical opened this issue Sep 18, 2018 · 24 comments

Comments

@ambihelical
Copy link

It would be nice if projectile offered some integration with project.el, as at least one package that I know of uses it to find projects. Basically provide a function that can be added to project-find-functions so that project.el can be aware of projectile projects.

I've done this for my own config by defining a couple of functions as follows. I can work up a PR if desired. The first finds the projectile project given a directory and is ripped from projectile because the functionality was not split out as a separate function:

;; function to use with project-find-functions
;; Find projectile project path associated with DIR
;; Code copied from projectile, should probably refactor projectile
;; to provide this.
(defun me:projectile-project-finder (dir)
  (cl-some
   (lambda (func)
     (let* ((cache-key (format "%s-%s" func dir))
            (cache-value (gethash cache-key projectile-project-root-cache)))
       (if (and cache-value (file-exists-p cache-value))
           cache-value
         (let ((value (funcall func (file-truename dir))))
           (puthash cache-key value projectile-project-root-cache)
           value))))
   projectile-project-root-files-functions))

The second adds that as a project.el root finder:

  (defun me:project-finder (dir)
    (if (boundp 'projectile-project-root-cache)
        (let ((root (me:projectile-project-finder dir)))
          (and root (cons 'transient root)))))

I then add project-finder to the list of project.el functions in the configuration of the package which uses project.el using
(add-to-list 'project-find-functions #'me:project-finder)

I think probably if projectile offered a similar function it would be sufficient, but it might not be bad if it was done when projectile is enabled, so it "just works".

@bbatsov
Copy link
Owner

bbatsov commented Sep 19, 2018

I think I looked into this once and I didn't like the integration points. The problem is that the way Projectile looks up projects it's hard to send the project data project.el needs (project dir + type). Projectile uses several functions in a row to find the root (I see in your snippet you've used just one out of them, but I guess the most common one), some of them overlapping with the built-in vc-project search that project.el does, and all functions return just the root, so you'll have to do extra query about the type (or change the base lookup functions).

@ambihelical
Copy link
Author

Basically the value is that there is at least one package (eglot) that uses project.el to find the root of projects. I use projectile to define my projects, so it is useful to "teach" project.el about projectile projects. That's basically all I am after with the integration.

I have to disagree with you on this point: the code in the first code snippet is copy-pasted from projectile, and potentially uses all of the functions set in projectile-project-root-files-functions, not just one. So it will find the project in the same way that projectile does even if the user has changed that list or any of the supporting file name lists.

There is also no issue with overlap. The function me:project-finder is tried first by project.el, if it fails, it reverts to its own finder functions. I've been using this code for a short time and haven't seen any other issues, not that there isn't any of course.

@bbatsov
Copy link
Owner

bbatsov commented Sep 19, 2018

Ah, yes. I misread something in the snippet. At any rate - I think this approach is flawed and the vc stuff should be removed any integration function. And we should also return some sensible project types instead of something generic I guess.

Basically the value is that there is at least one package (eglot) that uses project.el to find the root of projects. I use projectile to define my projects, so it is useful to "teach" project.el about projectile projects. That's basically all I am after with the integration.

Of course, you can also argue that it'd be better to teach eglot about Projectile. I know project.el is supposed to be the "standard", but anything that goes into Emacs generally grows stale due to the painful contribution process. I've always envisioned Projectile as an universal project library as well, so I'd rather have people use it directly instead of over some extremely limited proxy layer. That being said - at some point I can look into some integration and returning richer project metadata.

      (and root (cons 'transient root)))))

Why transient, btw?

I think probably if projectile offered a similar function it would be sufficient, but it might not be bad if it was done when projectile is enabled, so it "just works".

Yep, that's what I want to do as well.

@ambihelical
Copy link
Author

"and the vc stuff should be removed any integration function" not sure what you mean here.

I returned transient because I didn't want to use 'vc because it's not necessarily true, and 'transient was the only other available option. I don't know enough about project.el to define one for projectile, nor what the ramifications are.

I suppose if the code that I broke out (the first function) were broken out in projectile, eglot could offer integration itself. This would just be projectile offering more functionality, it has this functionality already it's just not a function you can call. I can make a PR for this, if that's acceptable.

@bbatsov
Copy link
Owner

bbatsov commented Sep 20, 2018

"and the vc stuff should be removed any integration function" not sure what you mean here.

The only function that project.el has itself is a VC project lookup and Projectile has the same functionality as well. Depending on the order of the hooks this might surprise someone, although in practice the differences are likely small.

I suppose if the code that I broke out (the first function) were broken out in projectile, eglot could offer integration itself. This would just be projectile offering more functionality, it has this functionality already it's just not a function you can call. I can make a PR for this, if that's acceptable.

I've tweaked projectile-project-root to accept an optional dir which solves this. I'll add something for project.el at some point.

I returned transient because I didn't want to use 'vc because it's not necessarily true, and 'transient was the only other available option. I don't know enough about project.el to define one for projectile, nor what the ramifications are.

To my knowledge it doesn't really matter what symbol you use. I'll likely use 'projectile in the beginning and then switch to something more specific (e.g. lein, make).

@ambihelical
Copy link
Author

I'll try out the new projectile-project-root. Thanks.

@ambihelical
Copy link
Author

It seems to be working, will test more extensively today. I also tried 'projectile instead of 'transient. It doesn't work because something (eglot?) calls the generic function project-roots, and there's no applicable function for 'projectile.

@ambihelical
Copy link
Author

Works fine with this in my eglot config:

  ;; project-find-function which uses projectile methods to find
  ;; the projectile project associated with a directory.
  ;; If projectile not loaded, or directory is not in a project,
  ;; hopefully returns nil.
  (defun me:project-finder (dir)
    (if (fboundp 'projectile-project-root)
        (let ((root (projectile-project-root dir)))
          (and root (cons 'transient root)))))
  (add-to-list 'project-find-functions #'me:project-finder)

@bbatsov
Copy link
Owner

bbatsov commented Sep 24, 2018

Yeah, that will return nil with master in the absence of a project. In 1.0, however, it will raise an error.

As for the transient stuff - that puzzles me, as I don't recall project.el defining any project types itself. Anyways, if that works - that's fine by me.

@bbatsov
Copy link
Owner

bbatsov commented Sep 24, 2018

I just read the code of project.el and I see that transient actually means there's no real project and there's no special handing for it. You might have found some bug with elgot. :-)

(defun project-current (&optional maybe-prompt dir)
  "Return the project instance in DIR or `default-directory'.
When no project found in DIR, and MAYBE-PROMPT is non-nil, ask
the user for a different directory to look in.  If that directory
is not a part of a detectable project either, return a
`transient' project instance rooted in it."
  (unless dir (setq dir default-directory))
  (let ((pr (project--find-in-directory dir)))
    (cond
     (pr)
     (maybe-prompt
      (setq dir (read-directory-name "Choose the project directory: " dir nil t)
            pr (project--find-in-directory dir))
      (unless pr
        (message "Using '%s' as a transient project root" dir)
        (setq pr (cons 'transient dir)))))
    pr))

(defun project--find-in-directory (dir)
  (run-hook-with-args-until-success 'project-find-functions dir))

@ambihelical
Copy link
Author

Transient projects seem to be good enough for eglot, although a 'projectile project type would be preferable. I can call project-xxx generic functions on transient projects, and get results, well except for project-files which for some reason isn't defined. Eglot apparently doesn't call that one, so it works out.

@bbatsov
Copy link
Owner

bbatsov commented Sep 24, 2018

@dgutov Does the type of the project have any significance in project.el? I really don't see anything like this in the code and for the integration logic I'd rather set some more meaningful project type. (You can read the just the final couple of comments here).

@ambihelical
Copy link
Author

I guess it is up to you, but I suppose there could be collisions, however unlikely. I believe it only selects the specialized functions you write for your project type, I don't know any other significance, but I am certainly no expert on anything elisp or emacs related.

@bbatsov
Copy link
Owner

bbatsov commented Sep 25, 2018

@dgutov is the author of project.el, that's why I pinged him. He should definitely know what's the best way to approach this. :-)

@bbatsov
Copy link
Owner

bbatsov commented Sep 25, 2018

Actually I took a look at eglot's code and it seems they are simply using transient as fallback for project-current (https://github.com/joaotavora/eglot/blob/821b4980caa0cc3379f0890c66cf05897229b0d5/eglot.el#L868)

Seems to me any project type should work with eglot then.

@dgutov
Copy link
Contributor

dgutov commented Sep 26, 2018

I know project.el is supposed to be the "standard", but anything that goes into Emacs generally grows stale due to the painful contribution process.

Or antagonistic external package maintainers who rather improve something that's within their control than contribute to the core. Which isn't particularly painful once one has signed the papers, which you and I have.

If you think I enjoy reading comments like this from you, and then helpfully answering questions, then no. I do not.

(You can read the just the final couple of comments here).

Really.

@dgutov
Copy link
Contributor

dgutov commented Sep 26, 2018

To answer the question, hopefully: no, you shouldn't use transient, or vc. Those are for "transient" and "vc" built-in project types, respectively.

To return both the project root, and the project type, either define a named structure type (using cl-defstruct), or even return a list ('projectile root type), and then dispatch on (head projectile). Example:

(cl-defmethod project-roots ((project (head projectile)))
  (list (nth 1 project)))

@bbatsov
Copy link
Owner

bbatsov commented Sep 26, 2018

Or antagonistic external package maintainers who rather improve something that's within their control than contribute to the core. Which isn't particularly painful once one has signed the papers, which you and I have.

@dgutov Ouch! If I offended you with my remarks - you've got my apologies. For the protocol - I've got nothing but respect for your work! 🙇 But I also don't understand why am I expected to abandon the project on which I've worked for 7 years and focus my efforts on project.el instead? (which can be one interpretation of your words 😉).

As for the Emacs contribution process - I wasn't referring to me, but to regular people who probably would contribute if the bar was let low, but would not bother otherwise. Lately I've been thinking that instead of adding things to Emacs we should be taking them out instead (to ELPA), so at least they can have a difference release cadence.

If you think I enjoy reading comments like this from you, and then helpfully answering questions, then no. I do not.

Sorry to hear this, but after all I'm asking because I want to add proper support your package. Which I completely fine with not doing, as I've got plenty of other things to work on.

From time to time I might make a snarky remark here and there when I'm in a bad mood or under a lot of stress, and once again apologies for this. I regret that I wasn't able to be more involved in project.el's earlier days and I don't discount the possibility of me doing more work there. It's just not something that's a priority for me, especially given how short on time I've been lately.

Anyways, I hope we're on good terms and thanks for your pointers!

@dgutov
Copy link
Contributor

dgutov commented Nov 20, 2018

Sorry for the late reply, but these kind of discussions are taxing and invariably leave me less happy than I was before participating.

I respect your work quite a bit, and please let me know if you ever see an undue criticism from me. For your own part, I'd rather wish you expressed the respect with actions. Or at least being willing to examine the design of the "competing" project. Which it's isn't, really (see below). Or at least, not yet.

One important thing I'd like everybody to see is that the hook-based design was explicitly chosen in acknowledgment that there are good, popular project management solutions (EDIT: for) Emacs already. Alas, since Projectile is not in the core, no core packages, functions, commands, etc, can use it directly. Hence the project-find-functions hook and the need for you to integrate with it.

The built-in VC backend is there because something has to be. I will continue experimenting with it (because why not), but it's really not the part you should be focusing on. Except for inspiration, maybe, if you find anything interesting in it.

@dgutov
Copy link
Contributor

dgutov commented Nov 20, 2018

Lately I've been thinking that instead of adding things to Emacs we should be taking them out instead (to ELPA), so at least they can have a difference release cadence.

It's a popular notion, and I support it, but we can't take everything out of Emacs. Something has to remain inside. And Emacs needs to remain usable without packages, too.

@dgutov
Copy link
Contributor

dgutov commented Nov 20, 2018

But I also don't understand why am I expected to abandon the project on which I've worked for 7 years and focus my efforts on project.el instead? (which can be one interpretation of your words wink).

A-a-and no.

The part where I'd like to see your contribution is the design of package.el API: the hook, the generic methods and their semantics. All from the point of view of Projectile maintainer, to make sure we didn't miss something, and didn't make them incompatible with Projectile in some important way.

@bbatsov
Copy link
Owner

bbatsov commented Nov 22, 2018

Sorry for the late reply, but these kind of discussions are taxing and invariably leave me less happy than I was before participating.

Same here. I've been out of my element lately and I vow to work towards changing this.

I respect your work quite a bit, and please let me know if you ever see an undue criticism from me. For your own part, I'd rather wish you expressed the respect with actions. Or at least being willing to examine the design of the "competing" project. Which it's isn't, really (see below). Or at least, not yet.

👍 Even if they were competing projects that'd be fine. Competition is a good thing! But so is collaboration and I certainly intend to collaboration with you and everyone else involved in project.el.

One important thing I'd like everybody to see is that the hook-based design was explicitly chosen in acknowledgment that there are good, popular project management solutions (EDIT: for) Emacs already. Alas, since Projectile is not in the core, no core packages, functions, commands, etc, can use it directly. Hence the project-find-functions hook and the need for you to integrate with it.

Yeah, that makes perfect sense.

The part where I'd like to see your contribution is the design of package.el API: the hook, the generic methods and their semantics. All from the point of view of Projectile maintainer, to make sure we didn't miss something, and didn't make them incompatible with Projectile in some important way.

👍

Thanks for you for your input!

@stale
Copy link

stale bot commented May 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the Stale label May 8, 2019
@stale
Copy link

stale bot commented Jun 7, 2019

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.

@stale stale bot closed this as completed Jun 7, 2019
laurynas-biveinis added a commit to laurynas-biveinis/dotfiles that referenced this issue Jan 10, 2020
laurynas-biveinis added a commit to laurynas-biveinis/dotfiles that referenced this issue Jan 10, 2020
laurynas-biveinis added a commit to laurynas-biveinis/dotfiles that referenced this issue Nov 26, 2022
Now that bbatsov/projectile#1282 and
bbatsov/projectile#1591 have been addressed in
bbatsov/projectile@a0105e7,
drop my own code to provide Projectile project root for xref and deadgrep.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants