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

project-switch-project stuck in current Clojure project #571

Closed
manuel-uberti opened this issue Sep 17, 2020 · 26 comments
Closed

project-switch-project stuck in current Clojure project #571

manuel-uberti opened this issue Sep 17, 2020 · 26 comments
Labels
bug third-party package Bugs related to third-party packages

Comments

@manuel-uberti
Copy link
Contributor

manuel-uberti commented Sep 17, 2020

Expected behavior

Once in a Clojure project, I am able to use C-x p p (project-switch-project) to select a different (non-Clojure) project and move there.

Actual behavior

C-x p p and the following f to find a file in another project display the files of the current Clojure project.

Steps to reproduce the problem

From emacs -Q, after having installed clojure-mode:

  • (require 'clojure-mode)
  • C-x p p
  • Pick a project (I chose a Git-versioned .emacs.d)
  • f
  • Type init.el and hit RET
  • C-x p p
  • Pick a Clojure project (I used my own boodle)
  • f
  • Pick a file (I chose deps.edn)
  • C-x p f
  • Type a random string (I typed aassdd) and hit RET
  • Hit C-g
  • C-x p p
  • Pick the other project (in my case, .emacs.d) and hit RET
  • f
  • You are now proposed to select a file from the Clojure project instead of the other one you selected when prompted by C-x p p

Environment & Version information

clojure-mode version

Probably it's because I am using straight but I get clojure-mode (version nil) when I use clojure-mode-display-version. In any case, I am on the commit 84ed16c * origin/master grafted Mention my ko-fi account.

System information

GNU Emacs 28.0.50

➤ Master branch commit:
92f342f38dd82aae4a662708dd6280fdfb2e013b
D-Bus: keep type information in D-Bus events

➤ Configured with the following options:
--with-harfbuzz
--with-json
--with-mailutils

➤ Running on:
Ubuntu 20.04 LTS
GNOME Shell 3.36.4 (x11)
GTK+ Version 3.24.18, cairo version 1.16.0

@bbatsov
Copy link
Member

bbatsov commented Sep 18, 2020

project.el is a bit light on documentation, so it's not very obvious what needs to be implemented exactly, other than project-roots. (if anything)

I can't really test this, as it seems it's something added in Emacs 27 or 28, and I'm still on 26. Not to mention I use only Projectile. 😆 Anyways, I'm assuming it won't be hard to fix.

Probably it's because I am using straight but I get clojure-mode (version nil) when I use clojure-mode-display-version. In any case, I am on the commit 84ed16c * origin/master grafted Mention my ko-fi account.

I guess you can file a separate ticket for this. I assumed this lm-version would work everywhere, but maybe that's not the case.

@bbatsov bbatsov added bug third-party package Bugs related to third-party packages labels Sep 18, 2020
@manuel-uberti
Copy link
Contributor Author

manuel-uberti commented Sep 18, 2020

I could try my way around this, but I'd probably need an input from @dgutov to understand the inner workings of project.el.
For instance: Is project-switch-project setting the project root every time a project is selected?

@bbatsov if you feel this discussion should be taken somewhere else, feel free to close this ticket and I'll see if I can find my way through some Emacs mailing list.

@bbatsov
Copy link
Member

bbatsov commented Sep 18, 2020

It's fine to keep the ticket open, but we definitely need more data points, and I don't have much time to investigate anything these days.

@manuel-uberti
Copy link
Contributor Author

FTR, I sent an email on emacs-devel just to avoid missing the core devs not active on GitHub: https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg01590.html

@muffinmad
Copy link

The clojure-project-dir function (used in the clojure-current-project project-find function) always returns cached dir value:

C-x C-f ~/workspace/testclojure/deps.edn
M-: (project-current) => (clojure . "~/workspace/testclojure/")
M-: (project-current nil "~/workspace/gittest") => (clojure . "~/workspace/testclojure/")

After swithing to *scratch* buffer:

M-: (project-current nil "~/workspace/gittest") => (vc . "~/workspace/gittest/")

Maybe clojure-project-dir must use buffer-local cached value only if the buffer file is a child of passed dir-name.

Or maybe add the clojure-current-project to the very end of project-find-functions.

@manuel-uberti
Copy link
Contributor Author

Thank you for looking into it. Those "maybes" are what I hope to clear out and understand. :)

@muffinmad
Copy link

The project-switch-project function just ask for directory, set default-directory to it and run one of the action functions. The action function (e.g. project-find-file) then call the project-current function to find the project in that default-directory by calling project-find-functions hooks.

Each functions on this hook is called in turn with one
argument, the directory in which to look, and should return
either nil to mean that it is not applicable, or a project instance.

I hope this answer you questions on emacs-devel ;)

@manuel-uberti
Copy link
Contributor Author

Great, thanks. But what is the best approach here, then? You found two possible solutions, but which one is the right thing to do?

@muffinmad
Copy link

IMO the clojure-project-dir must pay attention to the directory argument before returning cached value.

@manuel-uberti
Copy link
Contributor Author

Oh, I did open an MR with your other suggestion though. :)

@muffinmad
Copy link

Oh, I did open an MR with your other suggestion though. :)

That way clojure-project-dir will still return the wrong result.

@manuel-uberti
Copy link
Contributor Author

Do you suggest not using clojure-cached-project-dir?

@muffinmad
Copy link

Do you suggest not using clojure-cached-project-dir?

As a quick workaround for the issue, yes.

@manuel-uberti
Copy link
Contributor Author

Let me updated the PR then.

@manuel-uberti
Copy link
Contributor Author

Wait, though. clojure-cached-project-dir is used elsewhere too. @bbatsov what should the best approach be in this case?

@muffinmad
Copy link

Wait, though. clojure-cached-project-dir is used elsewhere too. @bbatsov what should the best approach be in this case?

Oh, I've misread the clojure-cache-project-dir. As a quick workaround for the issue you can disable project dir caching by customizing the clojure-cache-project-dir variable.

@manuel-uberti
Copy link
Contributor Author

And there is also the defcustom clojure-cache-project-dir than can be set to nil. With that to nil in my Emacs configuration and the code from the PR, I get the expected results.

@manuel-uberti
Copy link
Contributor Author

Turns out the PR is not needed at all, because setting clojure-cache-project-dir to nil is enough.

@bbatsov it's up to you now. If you think appending clojure-current-project to project-find-functions is still a nice thing to do, I can rephrase the PR accordingly. If not, feel free to close it. :)

@bbatsov
Copy link
Member

bbatsov commented Sep 18, 2020

And there is also the defcustom clojure-cache-project-dir than can be set to nil. With that to nil in my Emacs configuration and the code from the PR, I get the expected results.

Note, that this might cripple CIDER in some cases, that's why the caching exists. I'll review this more carefully later on.

@dgutov
Copy link
Contributor

dgutov commented Sep 18, 2020

@bbatsov

project.el is a bit light on documentation, so it's not very obvious what needs to be implemented exactly, other than project-roots. (if anything)

Have you seen the latest version of Commentary? There is no project-roots anymore, to begin with.

Check it out here: https://elpa.gnu.org/packages/project.html

@dgutov
Copy link
Contributor

dgutov commented Sep 18, 2020

Note, that this might cripple CIDER in some cases

CIDER uses project.el?

@bbatsov
Copy link
Member

bbatsov commented Sep 18, 2020

Nope, it uses the clojure-mode project root function a lot, which lead to the need for adding caching to it.

@manuel-uberti
Copy link
Contributor Author

Note, that this might cripple CIDER in some cases, that's why the caching exists. I'll review this more carefully later on.

Interesting. If that's the case, than something along the lines of appending clojure-current-project to project-find-functions could be a solution. Although, as @muffinmad pointed out, the value of clojure-project-dir would still be wrong.

@dgutov
Copy link
Contributor

dgutov commented Sep 18, 2020

Can I perhaps suggest to remove the integration with project.el from clojure-mode? As mentioned in the Commentary I linked to above, it probably creates more problems than it solves.

And for users who don't employ Git or Hg in their projects, there could be a documented manual way to set up that integration.

@bbatsov
Copy link
Member

bbatsov commented Sep 18, 2020

Have you seen the latest version of Commentary? There is no project-roots anymore, to begin with.

Saw it just now. Looks pretty good.

Can I perhaps suggest to remove the integration with project.el from clojure-mode? As mentioned in the Commentary I linked to above, it probably creates more problems than it solves.

I was thinking exactly the same thing. The clojure-mode logic makes sense to for tools like CIDER, but it's probably an overkill in general.

@manuel-uberti
Copy link
Contributor Author

That would make my PR obsolete, you evil man! ;)

Do you want me to close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug third-party package Bugs related to third-party packages
Projects
None yet
Development

No branches or pull requests

4 participants