search

clojure-emacs / clj-refactor.el

A CIDER extension that provides powerful commands for refactoring Clojure code.
GNU General Public License v3.0
771 stars 111 forks source link

Change cljr-add-project-dependency description #557

Closed velios closed 7 months ago

velios commented 7 months ago

Fix for https://github.com/clojure-emacs/clj-refactor.el/issues/547

velios commented 7 months ago

I don’t fully understand what happened, but I probably can’t offer a fix because I’m not authorized somewhere

vemv commented 7 months ago

Hi! Thanks for the PR.

I don’t fully understand what happened, but I probably can’t offer a fix because I’m not authorized somewhere

What do you mean?

velios commented 7 months ago

What do you mean?

image

This is the first time I've seen such a warning, but I sure you know what to do to merge it if you find PR useful

vemv commented 7 months ago

Thanks!

That warn is natural since you don't have write access to this repo.

Do you have any pending commit to push?

vemv commented 7 months ago

...note that the original issue suggests:

Also could maybe improve the error message a little in the case where it doesn't find the project file

which seems reasonable to me. Let me know if you'd like to give if a shot - I can assist if appreciated.

velios commented 7 months ago

which seems reasonable to me. Let me know if you'd like to give if a shot - I can assist if appreciated.

Yes, thank you for the opportunity. I tried to solve the problem and updated this PR.

While solving this problem, I saw the following piece of code. Now I'm not sure it's correct to specify any specific dependency management systems. If I understand correctly, five different systems are supported. I don’t know whether it’s correct to list all five or use the impersonal word "project".

For now I've settled on. Add a dependency to the project dependencies file. I'd like to discuss whether this could be phrased better? Perhaps I should have left more detailed information for the user. I will wait for your opinion to resolve this. Add a dependency to the project dependencies file(supported deps.edn, project.clj, build.boot, shadow-cljs.edn and pom.xml). 

(defun cljr--project-file ()
  (let ((project-dir (cljr--project-dir)))
    (or (let ((file (expand-file-name "project.clj" project-dir)))
          (and (file-exists-p file) file))
        (let ((file (expand-file-name "build.boot" project-dir)))
          (and (file-exists-p file) file))
        (let ((file (expand-file-name "deps.edn" project-dir)))
          (and (file-exists-p file) file))
        (let ((file (expand-file-name "shadow-cljs.edn" project-dir)))
          (and (file-exists-p file) file))
        (let ((file (expand-file-name "pom.xml" project-dir)))
          (and (file-exists-p file) file)))))
velios commented 7 months ago

@vemv I'm sorry for the bothering you. Could you look at the resulting solution?

velios commented 7 months ago

@vemv I'm sorry. I will not complete this PR. I don't feel confident enough when programming in elisp. This was intended as a small change to the text variable. If anyone wants to bring this to a finished state, feel free to use my PR as a basis.