Skip to content

Fix auto-insertion of spaces in paredit #566

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

Merged
merged 9 commits into from
Apr 19, 2020

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Apr 18, 2020

Fixes #565

The first 3 commits should be squashed together, I just found the original function hard to understand and had to refactor it along the way and then remove what seemed like uneccessary checks.


Before submitting a PR mark the checkboxes for the items you've done (if you
think a checkbox does not apply, then leave it unchecked):

  • The commits are consistent with our [contribution guidelines][1].
  • You've added tests (if possible) to cover your change(s). Bugfix, indentation, and font-lock tests are extremely important!
  • You've run M-x checkdoc and fixed any warnings in the code you've written.
  • You've updated the changelog (if adding/changing user-visible functionality).
  • You've updated the readme (if adding/changing user-visible functionality).

clojure-mode.el Outdated
(not (memq delim '(?\" ?{ ?\( )))
(not (or (derived-mode-p 'clojure-mode)
(not (memq delim '(?\" ?\{ ?\())) ;; always insert for [
(not (or (derived-mode-p 'clojure-mode) ;; FIXME why does this check exist
Copy link
Member

Choose a reason for hiding this comment

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

Because without it this code would be applied to any mode where paredit is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I thought it was a mode local variable - will add it back in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, wouldn't it make more sense to do (make-local-variable 'paredit-space-for-delimiter-predicates) inside clojure-paredit-setup rather than add global predicates?

Copy link
Member

Choose a reason for hiding this comment

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

I guess that's an option as well.

(describe "Interactions with Paredit"
;; reuse existing when-refactoring-it macro
(describe "it should insert a space"
(when-refactoring-it "before lists"
Copy link
Member

Choose a reason for hiding this comment

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

I get the point of using this macro here, but probably we need some alias for it, otherwise the tests read a bit weird. Probably not a big deal, though, as I can't suggest a great name right now - something like with-edits/actions.

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 couldn't think of a name as well, but at least the console output makes sense -

Interactions with Paredit:
  it should insert a space
    before lists (0.19ms)
    before vectors (0.17ms)
    before maps (0.16ms)
    before strings (0.15ms)
    after gensym (0.20ms)
    after symbols ending with ' (0.19ms)
  it should not insert a space
    for anonymous fn syntax (0.19ms)
    for hash sets (0.17ms)
    for regexes (0.16ms)
    for quoted collections (0.17ms)
    for reader conditionals (0.17ms)

@yuhan0
Copy link
Contributor Author

yuhan0 commented Apr 18, 2020

I did some more refactoring, realising that the other function clojure-no-space-after-tag was mostly repeated logic, and could be folded into a single predicate.

clojure--collection-tag-regexp doesn't seem to be used elsewhere so I made it obsolete too.
Side note: it also had the bug of using a naive "[a-zA-Z0-9]" regex which does not adequately describe clojure symbols. I noticed a few other regexes in the file which have a similar issue, but that's probably best left for another PR

@yuhan0
Copy link
Contributor Author

yuhan0 commented Apr 18, 2020

Also I'm not a Paredit user myself (which is why this issue originally went unnoticed), so it would be great if someone could help test these changes out!

@bbatsov bbatsov merged commit a7ba83c into clojure-emacs:master Apr 19, 2020
@bbatsov
Copy link
Member

bbatsov commented Apr 19, 2020

I noticed a few other regexes in the file which have a similar issue, but that's probably best left for another PR

Agreed.

Also I'm not a Paredit user myself (which is why this issue originally went unnoticed), so it would be great if someone could help test these changes out!

I guess you're into smartparens or lispy, right? Given the unit tests I'm reasonably confident with your changes, so I'll just merge them and see what feedback we'll get. I'm using paredit, but I guess I don't update my Emacs packages often enough if I didn't notice this myself. :D

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.

paredit-mode inserts extra space in clojure-mode
2 participants