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 WARNS check. #81

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

Add WARNS check. #81

wants to merge 2 commits into from

Conversation

rpgoldman
Copy link
Contributor

The SIGNALS check can correctly detect whether a block of code signals
a warning, but it also aborts the code block's execution. That's
appropriate for checking for an error, but not for a warning. This new
check confirms that a warning of a particular class is raised, a la
SIGNALS, but also allows the block of code in its scope to complete,
which may be more natural for some cases.

matthewdehaven pushed a commit to shop-planner/shop3 that referenced this pull request Aug 17, 2023
There is an open pull request to add the `warns` check to fiveam. If
that pull request is merged, the `warns` check here should no longer
be used. The check added by this commit will ensure that the `warns`
defined in warns-check.lisp is only used if `warns` is not already
defined.

See lispci/fiveam#81
The SIGNALS check can correctly detect whether a block of code signals
a warning, but it also aborts the code block's execution. That's
appropriate for checking for an error, but not for a warning. This new
check confirms that a warning of a particular class is raised, a la
SIGNALS, but also allows the block of code in its scope to complete,
which may be more natural for some cases.
@sionescu
Copy link
Member

Good idea, but it makes more sense to modify SIGNALS and instead have it abort only when the condition is a subtype of CL:ERROR.

@rpgoldman
Copy link
Contributor Author

If you would like such a mod, I could do it. I didn't do it before because changing SIGNALS would not be backward-compatible -- it could break users' previously existing tests -- but WARNS would not because it's wholly new.

Let me know what you would prefer, and I will do one of the other.

@sionescu
Copy link
Member

sionescu commented Sep 1, 2023

Yes, I prefer to modify SIGNALS. It could break existing tests, but I think this is a fairly edge case that is not worth preserving.

SIGNALS now captures the return value of the signaling block, EXCEPT
when the condition signaled is an error, in which case there may not
be a value to return.

Changes in tests.lisp illustrate the trade-offs.
@rpgoldman
Copy link
Contributor Author

I have a new version of this, but it turned into a kludge, because it required some DWIMing.

The reason that the previously-existing signals macro does not return the value of the signaling expression is that that value may not be well-defined, especially when the condition signaled is a subclass of error.

So my DWIMed-up version of signals has the following 2 special cases:

  1. If the condition signaled is an error, then we simply return nil from the test-expression, as the original version of signals did.
  2. If the condition is a warning, we muffle that warning and continue.

In all cases except error, signals returns all values returned by evaluating its body.

Honestly, I think what would have been best would be either to pull the error special case out of signals and use signals only for conditions that continue, keep the original proposal and just add warns, or add an argument to signals to indicate whether you want the value of the body form (this last seems awfully fussy).

I have pushed the latest, kludgy version to this PR so you can look it over and make a choice.

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