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 imenu with Clojure code in string or comment #638

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

dakra
Copy link
Contributor

@dakra dakra commented Oct 26, 2022

Ignore error that's thrown from down-list when called
with point inside a string or comment.

E.g. with code like:

(defn foo []
  (let [a "
(defn b [_]
  (bar {:bla \"bla\"}))"]))

clojure-match-next-def calls down-list with point inside
the string and down-list will throw an user-error with
"This command doesn't work in strings or comments".


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.
  • 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).

Thanks!

@dakra
Copy link
Contributor Author

dakra commented Dec 3, 2022

Friendly ping.
Normally would not bump so early but I saw recent activity here and thought this probably just got overlooked by mistake.
If not, all good, take your time :)

@bbatsov
Copy link
Member

bbatsov commented Dec 3, 2022

Can you push again? I see something was broken with the CI back then and I guess that's why I didn't review this PR.

@@ -776,7 +776,8 @@ Called by `imenu--generic-function'."
(let (found?
(deftype (match-string 2))
(start (point)))
(down-list)
(ignore-errors
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 some comment here would be useful, otherwise it's really hard to tell why this error needs to be ignored. I'm also a bit worried this might suppress actual errors and complicate the debugging.

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 looked a bit more into it why I didn't happen before and nobody noticed.
The change to down-list came only in May with this: https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=0b3b295776ce723885c9997ab26d57314db2a5df

I guess a better workaround would be to wrap it in
unless (ppss-comment-or-string-start (syntax-ppss)) instead of ignore-errors,
but ppss-comment-or-string-start is only available since Emacs 27 and clojure-mode
is marked to support Emacs 25+.

I updated the commit message with this info and added a short comment.
Happy to rephrase it or change something else.

Copy link
Member

Choose a reason for hiding this comment

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

If ppss-comment-or-string-start is not complex I guess we can just copy it and inline it.

CHANGELOG.md Outdated Show resolved Hide resolved
@dakra dakra force-pushed the fix-imenu branch 4 times, most recently from 11261f0 to 83c4103 Compare December 3, 2022 11:12
@dakra
Copy link
Contributor Author

dakra commented Dec 3, 2022

CircleCI still red because it couldn't find a usable config.yml

@bbatsov
Copy link
Member

bbatsov commented Dec 11, 2022

Ah, CircleCI is killing me these days... Let me try resetting the build token once again. @vemv (or anyone else) - if someone has better ideas what to do about this build issue let me know.

@bbatsov
Copy link
Member

bbatsov commented Dec 11, 2022

@dakra You can try pushing again to see if now the CI will work. I plan to cut a new release soon and it'd be nice for this to make it there.

Ignore error that's thrown from `down-list` when called
with point inside a string or comment.

E.g. with code like:

```
(defn foo []
  (let [a "
(defn b [_]
  (bar {:bla \"bla\"}))"]))
```

`clojure-match-next-def` calls `down-list` with point inside
the string and `down-list` will throw an user-error with
"This command doesn't work in strings or comments".

This user-error in `down-list` got introduced 2022-05-06
with https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=0b3b295776ce723885c9997ab26d57314db2a5df

The `when ignore-errors` could be replaced with
`unless (ppss-comment-or-string-start (syntax-ppss))`
once the minimum requirement for clojure-mode is Emacs 27.
@dakra
Copy link
Contributor Author

dakra commented Dec 11, 2022

@bbatsov done. but CircleCI instantly failed again :/

@bbatsov
Copy link
Member

bbatsov commented Dec 12, 2022

@vemv Any idea what's wrong with CircleCI these days? I'm getting progressively more frustrated with it, given the recent OAuth issues that happened for no apparent reason.

@bbatsov bbatsov merged commit fe29a03 into clojure-emacs:master Dec 14, 2022
@dakra dakra deleted the fix-imenu branch March 17, 2023 11:36
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