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

Adding catch_panic anti-pattern #109

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Conversation

simonsan
Copy link
Collaborator

@simonsan simonsan commented Jan 1, 2021

Due to inactivity of the author and maintainers not being able to edit the PR I'll move #22 to a new branch in this repository.

Future fixes to the PR will be made on this branch.

@simonsan simonsan added the C-addition Category: Adding new content, something that didn't exist in the repository before label Jan 1, 2021
@simonsan simonsan force-pushed the catch_panic_anti_pattern branch from da9e732 to 9c738a6 Compare January 1, 2021 12:50
@pickfire
Copy link
Contributor

pickfire commented Jan 1, 2021

Isn't web frameworks doing this? Is this really an anti-pattern?

@MarcoIeni
Copy link
Collaborator

Isn't web frameworks doing this? Is this really an anti-pattern?

In the "advantages" section it is mentioned

@MarcoIeni
Copy link
Collaborator

We can take a lot from the official documentation: https://doc.rust-lang.org/std/panic/fn.catch_unwind.html

anti_patterns/catch_panic.md Outdated Show resolved Hide resolved
@simonsan simonsan force-pushed the catch_panic_anti_pattern branch from 33674b9 to 8871ccb Compare January 6, 2021 09:56
@simonsan simonsan force-pushed the catch_panic_anti_pattern branch from b61bee8 to 9e2651a Compare January 6, 2021 14:31
@simonsan
Copy link
Collaborator Author

simonsan commented Jan 6, 2021

@MarcoIeni
Why is it expecting a + now for ul? :D

anti_patterns/catch_panic.md:76:1 MD004/ul-style Unordered list style [Expected: plus; Actual: dash]
anti_patterns/catch_panic.md:77:1 MD004/ul-style Unordered list style [Expected: plus; Actual: dash]
anti_patterns/catch_panic.md:78:1 MD004/ul-style Unordered list style [Expected: plus; Actual: dash]
anti_patterns/catch_panic.md:79:1 MD004/ul-style Unordered list style [Expected: plus; Actual: dash]

@MarcoIeni
Copy link
Collaborator

By default it expects the style to be consistent, see this

Since there is a + on line 66 it expects that all the lists follow that.

@simonsan
Copy link
Collaborator Author

simonsan commented Jan 6, 2021

By default it expects the style to be consistent, see this

Since there is a + on line 66 it expects that all the lists follow that.

ah ok, then it's nice!

just didn't see the plus and the error message was not so helpful as i'm used to from rustc :D

@pickfire
Copy link
Contributor

We only see what's bad, is there something that could be done instead of this pattern, if there is maybe we should show it.

@simonsan
Copy link
Collaborator Author

simonsan commented Jan 11, 2021

We only see what's bad, is there something that could be done instead of this pattern, if there is maybe we should show it.

As this anti-pattern is focussed on "using a method that catches unexpected problems (implementation bugs) for handling an expected problem, such as a missing file.", Imho use good error handling is the answer to this? What would you say?

EDIT: So a link to the error handling chapter on the rust book could be sufficient? Not sure, but open for your ideas. :)

@pickfire
Copy link
Contributor

I think that is an option but at least not so easy to pickup, but at least we have some suggestions. I am not aware of any straightforward switch.

@simonsan simonsan added the A-anti_pattern Area: Content about Anti-Patterns label Jan 23, 2021
@simonsan simonsan added the C-stale Category: An issue/a PR that hasn't been worked on in a longer time (usually 90 days) label Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-anti_pattern Area: Content about Anti-Patterns C-addition Category: Adding new content, something that didn't exist in the repository before C-stale Category: An issue/a PR that hasn't been worked on in a longer time (usually 90 days)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants