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

[Fix #2941] Use main args in :cider/nrepl alias for clojure cli #2970

Merged

Conversation

dpsutton
Copy link
Contributor

Addresses #2941.

Hopefully the string building is a bit more clear with the format string

    (format "%s-Sdeps '{:deps {%s} :aliases {:cider/nrepl {:main-opts [%s]}}}' -M:cider/nrepl"
            (if global-opts (format "%s " global-opts) "")
            deps-string
            main-opts)

@dpsutton
Copy link
Contributor Author

weird. i'm not seeing any CI. tests pass for me and "Linter has no complaints"

cider.el Outdated
(cider-jack-in-normalized-nrepl-middlewares)
",")
"]")))
('clojure-cli cider-clojure-cli-parameters)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this go away? i don't think it serves any purpose now. not sure the best cleanup of it. all the interesting stuff (refactor, etc) go in other places and the cider-clojure-cli-global-options is where you pass -A:dev:test and the like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it should go away. In general I still like the idea to simplify the config for each tool instead of just copying the original config for Lein everywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, if we're using the global options just for aliases, maybe we should rename it to something like cider-clojure-cli-aliases or whatever. I guess this might make things easier for the end users.

@bbatsov
Copy link
Member

bbatsov commented Jan 23, 2021

weird. i'm not seeing any CI. tests pass for me and "Linter has no complaints"

Perhaps we need to change something in the config? No idea why the CI results are not shown in the PR. I haven't changed the project config in ages.

:group 'cider
:safe #'stringp
:package-version '(cider . "0.17.0"))
(make-obsolete-variable 'cider-clojure-cli-global-options 'cider-clojure-cli-aliases "1.1")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbatsov wasn't sure what version to put here.

@dpsutton
Copy link
Contributor Author

I think all i need from you is what version to put in the make-obsolete-variable and then to squash and we're good to go.

@bbatsov
Copy link
Member

bbatsov commented Jan 25, 2021

The next release is going to be 1.1.

@@ -157,23 +157,15 @@ default to \"powershell\"."
:safe #'stringp
:package-version '(cider . "0.17.0"))

(defcustom cider-clojure-cli-global-options
(defcustom cider-clojure-cli-aliases
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should update the docstring as well. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. thanks

@@ -126,7 +126,7 @@ You can further customize the command line CIDER uses for `cider-jack-in` by
modifying the following string options:

* `cider-lein-global-options`, `cider-boot-global-options`,
`cider-clojure-cli-global-options`, `cider-gradle-global-options`:
`cider-clojure-cli-aliases`, `cider-gradle-global-options`:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And you should remove the params a couple of lines later. Now that the params are a bit different it might be a good idea to just list the params for each build tools separate (e.g. in subsections named "Leiningen", "Boot", etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which params should be removed? Not sure i'm following.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cider-clojure-cli-parameters is mentioned in the next paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh totally. I was thinking this was in the code file cider.el and was super confused. Gotcha

@bbatsov bbatsov merged commit 991cd3f into clojure-emacs:master Jan 25, 2021
@bbatsov
Copy link
Member

bbatsov commented Jan 25, 2021

Thanks!

@dpsutton dpsutton deleted the 2941-use-profiles-for-main-opts branch January 25, 2021 16:36
@Bost
Copy link

Bost commented Jan 27, 2021

I had in my .dir-locals.el

((clojure-mode . ((cider-preferred-build-tool . clojure-cli)
                  (cider-clojure-cli-global-options . "-J-Djdk.attach.allowAttachSelf"))))

Changing it to cider-clojure-cli-aliases doesn't work. cider-clojure-cli-parameters doesn't work either.

@dpsutton
Copy link
Contributor Author

@Bost did you update? if you do m-x describe-variable [enter] cider-clojure-cli-parameters is the variable present? If so, does it say its current value due to the dir-locals? If not, try m-: (hack-local-variables) to try to make it pick up the new values.

image

then when i jack in its using -M:test:cider/nrepl:
image

;; Startup: /usr/local/bin/clojure -Sdeps '{:deps {nrepl/nrepl {:mvn/version "0.8.3"} cider/cider-nrepl {:mvn/version "0.25.8"}} :aliases {:cider/nrepl {:main-opts ["-m" "nrepl.cmdline" "--middleware" "["cider.nrepl/cider-middleware"]"]}}}' -M:test:cider/nrepl

@dpsutton
Copy link
Contributor Author

ah dang. i just read through your usecase there. I don't think my changes work for that. I forgot about jvm parameters like that. I'll need to work something out for your use case. I'm really sorry about that.

@dpsutton
Copy link
Contributor Author

the easiest workaround for you right now is to jack in with a prefix C-u C-c M-j and throw that string into the jack in command at the beginning. I had forgotten about those types of parameters.

@dpsutton
Copy link
Contributor Author

dpsutton commented Jan 27, 2021

@Bost so sorry about that. These changes should fix it for you. #2974

@Bost
Copy link

Bost commented Jan 27, 2021

Not a big deal. Thanks for fixing it quickly! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants