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

Add :no-enclosing-expression message #1161

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

Conversation

tomdl89
Copy link

@tomdl89 tomdl89 commented May 8, 2023

I noticed quite a few commands fail with a generic error when point is not enclosed by a sexp. E.g. call sp-forward-barf-sexp when not inside parens, and you will see:

Wrong type argument: number-or-marker-p, nil

Because many commands call sp-get-enclosing-sexp which can return nil, and then subsequent operations try to do e.g. math with nil resulting in the above sort of error.

This PR adds a new sp-message error and tries to display that rather than the generic error, where possible. I gave up trying to run tests locally, but happy to be instructed if it's not too much effort :)

@Fuco1
Copy link
Owner

Fuco1 commented May 29, 2023

The code which is wrapped with -when-let or when ok shouldn't give any errors because the body won't execute. Where it does, it probably misses the check and there it might be reasonable to handle it.

In non-interactive commands, we shouldn't display any errors because they might not be called by the user. Some of the functions can be called both interactively and non-interactively and this is quite a mess to handle.

Rather than adding the messages, I would handle the nil errors only.

@tomdl89
Copy link
Author

tomdl89 commented May 29, 2023

Hey @Fuco1 thanks for your reply. I'm not sure I understand it entirely.

  • I agree that the code wrapped with -when-let or when ok shouldn't give any errors because the body won't execute, but it does, so that's why I added this PR. Are you suggesting the underlying error be fixed rather than showing an error message? I think calling these commands not inside an enclosing expression is kind of undefined behaviour, so an error does seem to make sense. In fact, this seems more like an argument against error messages in general, no?
  • All the functions I added this to (apart from sp--next-thing-selection which was displaying this message anyway) are "interactive commands", but I agree messages shouldn't be displayed when not called interactively. Adding to sp-message a check via called-interactively-p could fix that across the board?
  • What do you mean by "I would handle the nil errors only"?

Thanks again

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

Successfully merging this pull request may close these issues.

None yet

2 participants