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

legit/porcelain: signal a lem:editor-error #1227

Closed
wants to merge 2 commits into from

Conversation

vindarel
Copy link
Collaborator

@vindarel vindarel commented Jan 3, 2024

As per your review on #1198

ERROR is treated as an editor bug when the command loop catches it.

> ERROR is treated as an editor bug when the command loop catches it.

and editor-error is more friendly.
(defun %editor-error (message &rest args)
"Signal a lem:editor-error condition if the :lem package exists, otherwise signal a simple error.
This helps avoiding a tight dependency on Lem in the porcelain package."
(let ((msg (apply #'format nil message args)))
Copy link
Member

Choose a reason for hiding this comment

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

uiop:symbol-call only fools the compiler and actually makes it dependent on lem.
Besides, this kind of pattern is troublesome when refactoring and should be avoided if possible.

I have a suggestion, how about the following?

  • define porcelain condition
  (define-condition porcelain-error (error) ...)
  • Call that porcelain-error on this line.
(error 'porcelain-error ...)
  • Each define-command handles porcelain-error, converts it to editor-error, and lifts it up.
(define-command legit-... (...) (...)
  (with-porcelain-error () ...))

or

(defclass legit-advice () ())

(defmethod execute (mode (command legit-advice) argument)
  (with-porcelain-error () (call-next-method)))

(define-command (legit-... :advice-classes legit-advice) () ())
  ...)

@vindarel vindarel closed this Jan 4, 2024
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