Skip to content

Conversation

@cblp
Copy link
Contributor

@cblp cblp commented Feb 10, 2024

No description provided.

@ocramz
Copy link
Collaborator

ocramz commented Feb 11, 2024

@cblp Thanks, but we can't merge code if it breaks CI. If you want to show a problem with the current implementation, could you make a dedicated subsection in the tests ("context"..) and rewrite tests such that they pass?

@cblp
Copy link
Contributor Author

cblp commented Feb 11, 2024

I can't make it pass without changing main implementation.

@cblp
Copy link
Contributor Author

cblp commented Feb 11, 2024

@ocramz what do you think of this implementation?

@ocramz ocramz self-requested a review February 22, 2024 06:18
Copy link
Collaborator

@ocramz ocramz left a comment

Choose a reason for hiding this comment

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

I would like some new tests that clarify the need for the new exception handler priority.

runAction mh env action = do
ok <- flip runReaderT env $ runAM $ tryNext $ action `catches` concat
[ [actionErrorHandler]
[ [actionErrorHandler, statusErrorHandler, scottyExceptionHandler]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes the behaviour of the library, but it's not caught in the tests.

@cblp
Copy link
Contributor Author

cblp commented Mar 10, 2024

I'm not sure how to write such tests, I did it intuitively. I just want to see exception when it is thown from a handler.

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