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

Handle exceptions from scotty as ScottyException #345

Merged
merged 1 commit into from
Dec 1, 2023
Merged

Conversation

fumieval
Copy link
Collaborator

@fumieval fumieval commented Oct 20, 2023

This change replaces the use of StatusError by ScottyException. The idea is to avoid returning a error response in an arbitrary format and make it more reasonable to catch them, because catching StatusError doesn't make much sense

For the sake of compatibility, I kept the StatusError type but I think it should eventually be deprecated.

closes #338

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.

It's a good change overall but I'd rather keep imports explicit and docstrings clearer.

Web/Scotty.hs Outdated Show resolved Hide resolved
Web/Scotty/Action.hs Outdated Show resolved Hide resolved
Web/Scotty/Action.hs Outdated Show resolved Hide resolved
Web/Scotty/Action.hs Outdated Show resolved Hide resolved
Web/Scotty/Trans.hs Outdated Show resolved Hide resolved
@fumieval fumieval force-pushed the scotty-exception branch 2 times, most recently from f78880d to ed8f3d0 Compare October 21, 2023 03:26
@fumieval
Copy link
Collaborator Author

@ocramz Do you have any remaining concerns?

@fumieval fumieval requested a review from ocramz November 2, 2023 09:09
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.

It's a good PR, but I still think captureParamMaybe doesn't need the MonadIO constraint.

Also, shouldn't we add a deprecation warning to raiseStatus?

Web/Scotty/Action.hs Outdated Show resolved Hide resolved
Web/Scotty/Action.hs Outdated Show resolved Hide resolved
@fumieval fumieval force-pushed the scotty-exception branch 2 times, most recently from 8f6162f to 6dd48a7 Compare November 5, 2023 09:49
@fumieval
Copy link
Collaborator Author

fumieval commented Nov 5, 2023

I'm going to deprecate raiseStatus in a separate PR.

@fumieval fumieval requested a review from ocramz November 7, 2023 08:03
@fumieval
Copy link
Collaborator Author

@ocramz ping

@ocramz
Copy link
Collaborator

ocramz commented Nov 21, 2023 via email

@fumieval
Copy link
Collaborator Author

@ocramz I see, great! Have a wonderful vacation

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.

Great, thank you! 👍

@ocramz ocramz self-requested a review November 30, 2023 17:15
@ocramz
Copy link
Collaborator

ocramz commented Nov 30, 2023

@fumieval could you add a Changelog entry about this as well please?

@fumieval
Copy link
Collaborator Author

fumieval commented Dec 1, 2023

@ocramz Sure, I updated the changelog

@ocramz ocramz merged commit a10ebf8 into master Dec 1, 2023
5 checks passed
@ocramz ocramz deleted the scotty-exception branch December 1, 2023 04:30
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.

'status' and 'raiseStatus' (and 'redirect') overlap in functionality
2 participants