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

introduced variable cider-clojure-cli-global-aliases #3623

Merged

Conversation

behrica
Copy link
Contributor

@behrica behrica commented Feb 25, 2024

This PR introduces a variable cider-clojure-cli-global-aliases
Discussed here: https://clojurians.slack.com/archives/C0617A8PQ/p1708857780583699

It allows a user to have a "global" system wide set of aliases, which get added by cider to each repl start.
It allows to combine "project specific aliases" and" global aliases"

@vemv
Copy link
Member

vemv commented Feb 25, 2024

Looks like a reasonable start, let's see what @bbatsov thinks as well

(note as future steps: there's test coverage for this area - easy to extend)

@bbatsov
Copy link
Member

bbatsov commented Feb 28, 2024

I get the intent, but I'm curious why is the current variable considered to be project-specific, given that nothing about it implies it. Might be better to actually add something name cider-clojure-cli-project-aliases instead that's meant to be used only via .dir-locals.el.

If we go with your approach the documentation for the existing defcustom will need to be updated. I also think this will need some coverage in the user manual.

@vemv
Copy link
Member

vemv commented Feb 28, 2024

Might be better to actually add something name cider-clojure-cli-project-aliases instead that's meant to be used only via .dir-locals.el.

This would work as well (it's basically the same endgame as proposed in this PR - two variables).

However over the last few years we've taught people to set cider-clojure-cli-aliases in .dir-locals.el (and/or that's what people do anyway), so changing the intended semantics for cider-clojure-cli-aliases would seem overly disruptive.

@bbatsov
Copy link
Member

bbatsov commented Feb 28, 2024

Yeah, that's fair. As I never used clojure-cli much, it's hard for me to assess whether people have more need for global aliases or project aliases (e.g. how like it is that those differ at all, as with many build system I have more or less the same setup across many projects)

@behrica
Copy link
Contributor Author

behrica commented Feb 29, 2024

I think that most people have need for 2 things:

  • global alias for some functions they want to have always available, in any project
  • project specific alias, like test , dev in slide variations of name

-> ideally you want be able to "add" them together
-> the globals, should usually be first (they do not define :main entry points, the add functions to ns 'user for example)

@vemv
Copy link
Member

vemv commented Feb 29, 2024

Agreed - that's what most people would expect. You have the green light for the PR as-is, simply follow the suggestions when you have the chance!

CHANGELOG.md Outdated Show resolved Hide resolved
cider.el Outdated Show resolved Hide resolved
cider.el Outdated Show resolved Hide resolved
@vemv
Copy link
Member

vemv commented Mar 27, 2024

Hi @behrica, feel free to polish this PR these days, would be good timing in preparation for the next major CIDER release.

cider.el Outdated

(defcustom cider-clojure-cli-global-aliases
nil
"A list of global aliases to include when using the clojure cli.
Copy link
Contributor

Choose a reason for hiding this comment

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

This wording (also the existing docstring for cider-clojure-cli-aliases) seems potentially confusing - one might mistake them for an actual '(":list" ":of" ":strings"). I'd suggest something like:

Global aliases to include when jacking in with the clojure CLI.
This value should be a string of the form ":foo:bar", and
will be prepended to the value of `cider-clojure-cli-aliases'.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also shouldn't the type technically be '(choice string (const nil))? I see the same empty-string nil-punning for several other defcustoms above, not sure how idiomatic this is in elisp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated comment as proposed.

Regarding the "type" I was not sure about it, so left it as is

@behrica behrica requested review from bbatsov April 7, 2024 20:13
@bbatsov bbatsov merged commit 6b60332 into clojure-emacs:master Apr 21, 2024
35 checks passed
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.

4 participants