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

Exception stringification followup #1426

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

Conversation

uuf6429
Copy link

@uuf6429 uuf6429 commented Apr 8, 2023

This is the follow up for #1421 (comment)

On a side note, I'm wondering if supportsException should also check for the stringification classes.
Although its name relates to the exception, the usage actually relates to if it can stringify an exception or not.

@ciaranmcnulty
Copy link
Contributor

Thanks @uuf6429 I like the idea to add an exception, but not a fan of the change to a switch/case

I'll try and look at the test suite and add something to the matrix that would have caught the issue before #1422 was merged, so we can check that this change is preserving the same behaviour

@uuf6429
Copy link
Author

uuf6429 commented Apr 12, 2023

but not a fan of the change to a switch/case

I find that it is very readable once you get used to the concept; it keeps the cause-and-effect very close.
My personal policy on the matter is that anything more than two if conditions (with direct returns and an else) is more suited to a switch.

I agree about a test, but I'm not sure how you can do such a test in practice (since it depends on (non-)existence of classes).

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.

None yet

2 participants