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

cider-ns - refinements #3632

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

cider-ns - refinements #3632

wants to merge 7 commits into from

Conversation

vemv
Copy link
Member

@vemv vemv commented Mar 10, 2024

  • Strengthen cider-ns--present-error
  • cider-ns--present-error: prefer end-of-sexp
    • One has to use structural movements in order to present the overlay without errors.
    • By using this finer-grained function, it's more likely that the overlay will be rendered at the right place (and not too far away at the bottom).
  • cider-ns-refresh: accept clear-and-inhibit mode
    • It often makes sense not to run the before/after functions around the clearing.

Cheers - V

vemv added 3 commits March 10, 2024 20:42
One has to use structural movements in order to present the overlay without errors.

By using this finer-grained function, it's more likely that the overlay will show at the right place (and not too far away at the bottom).
cider-ns.el Outdated Show resolved Hide resolved
@@ -286,24 +286,27 @@ Uses the configured 'refresh dirs' \(defaults to the classpath dirs).
With a single prefix argument, or if MODE is `refresh-all', reload all
namespaces on the classpath dirs unconditionally.

With a double prefix argument, or if MODE is `clear', clear the state of
the namespace tracker before reloading. This is useful for recovering from
With a double prefix argument, or if MODE is `clear' (or `clear-and-inhibit'),
Copy link
Member

Choose a reason for hiding this comment

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

Probably we'll need to document this somewhere in the manual as well, otherwise few people are going to find it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. While we're here, should the new mode be invokable through some prefix incantation?

tbh I don't use prefixes at all so I wouldn't know how to add a new variation here.

Copy link
Member

Choose a reason for hiding this comment

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

That would be nice indeed. I think multiple prefixes had to be checked as some number value in the code to differentiate them. There must be some examples in the code. I do think, however, we should start moving towards transient for prefix arguments as it's UI is so much better, and it's bundled with Emacs 28+.

Copy link
Member Author

Choose a reason for hiding this comment

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

I expanded the user manual

I do think, however, we should start moving towards transient for prefix arguments as it's UI is so much better

In face of this maybe we can refrain from doubling down on prefixes (at least here).

Transient rocks and would love to see a few features / examples of how it would look for CIDER

Copy link
Member Author

@vemv vemv left a comment

Choose a reason for hiding this comment

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

Ready again with a cider--semantic-end-of-line which seems to do the right thing

@@ -286,24 +286,27 @@ Uses the configured 'refresh dirs' \(defaults to the classpath dirs).
With a single prefix argument, or if MODE is `refresh-all', reload all
namespaces on the classpath dirs unconditionally.

With a double prefix argument, or if MODE is `clear', clear the state of
the namespace tracker before reloading. This is useful for recovering from
With a double prefix argument, or if MODE is `clear' (or `clear-and-inhibit'),
Copy link
Member Author

Choose a reason for hiding this comment

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

I expanded the user manual

I do think, however, we should start moving towards transient for prefix arguments as it's UI is so much better

In face of this maybe we can refrain from doubling down on prefixes (at least here).

Transient rocks and would love to see a few features / examples of how it would look for CIDER

@@ -1,4 +1,5 @@
edn
fo
Copy link
Member

Choose a reason for hiding this comment

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

Why so? That's often a misspelled of.

@bbatsov
Copy link
Member

bbatsov commented Mar 14, 2024

I'll take a closer look tomorrow.

@@ -826,6 +826,28 @@ KIND can be the symbols `ns', `var', `emph', `fn', or a face name."
(t x)))
menu-list))

(defun cider--semantic-end-of-line ()
"Returns POINT at the closest EOL that we can move to semantically,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this. What's an EOL to which we can move semantically? I checked the tests as well, but all assertions are in a single tests, so it's pretty hard to figure out what you're testing for exactly.

@@ -62,17 +62,24 @@ Typing kbd:[C-c M-n r] or kbd:[C-c M-n M-r] will invoke
`cider-ns-refresh` and reload all modified Clojure files on the
classpath.

Adding a prefix argument, kbd:[C-u C-c M-n r], will reload all
Adding a prefix argument, kbd:[C-u C-c M-n r], it will reload all
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 "Adding" is not the right terminology here, which affects the rest of the sentence (which reads a bit awkwardly to me). I'd write something like "If you invoke cider-ns-refresh with a prefix argument it will" or something along those lines.

@bbatsov
Copy link
Member

bbatsov commented Apr 21, 2024

Ping :-)

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.

2 participants