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 better error message when network lock is held #274

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mitchellwrosen
Copy link
Collaborator

This PR adds a better error message when attempting to acquire the network lock while it is held.

I think this partially addresses #190, though we could/should also add documentation to execute to call out this behavior.

To do:

  • Decide on an exception type. This currently uses fail in IO, so IOException. I find this kind of nasty and old-school, but this exception shouldn't really ever be caught and handled in any particular way.
  • Nail down exact wording of error message
  • Update execute documentation (and are there other common ways this can occur?)

@mitchellwrosen
Copy link
Collaborator Author

Any suggestions for the exception type or what the error message should say?

@HeinrichApfelmus
Copy link
Owner

Decide on an exception type. This currently uses fail in IO, so IOException. I find this kind of nasty and old-school, but this exception shouldn't really ever be caught and handled in any particular way.

The standard style in the Control.Exception module is to define a single type for each exceptional outcome. Here is what I would suggest:

-- | Thrown when an event handler is fired from within a `Network`
-- while handling events. This usage does not have well-defined semantics.
--
-- Typically, this happens when `fire` is called during `execute`.
-- You may want to use `reactimate` instead of `execute`.
--
-- If really want to `fire` an event from within `execute`,
-- you need to add a delay;
-- `liftIOLater` can be helpful in this situation.
--
-- Example: […]
data NestedEventHandling = NestedEventHandling

instance Exception NestedEventHandling

Nail down exact wording of error message

I would take a subset of the comment documenting the NestedEventHandling exception and simply refer to the exception.

Update execute documentation (and are there other common ways this can occur?)

👍 Referring to the documentation of the exception is probably the right way to go. I don't think that there is any other way — reactimate can use fire, if I remember correctly.

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