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

Generalize exception type #951

Merged
merged 1 commit into from
Aug 21, 2021
Merged

Generalize exception type #951

merged 1 commit into from
Aug 21, 2021

Conversation

timholy
Copy link
Member

@timholy timholy commented Aug 19, 2021

In preparation for JuliaLang/julia#41885

Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Looks good to me, pending this getting merged upstream.

@test_throws(
"reducing over an empty collection is not allowed",
mapreduce(nothing, nothing, SVector{0,Int}())
)
Copy link
Member

Choose a reason for hiding this comment

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

For anyone (like myself) thinking this use of @test_throws looks confusing... see JuliaLang/julia#41888

@timholy
Copy link
Member Author

timholy commented Aug 20, 2021

pending this getting merged upstream.

That's of course fine, but the fact that it returned an ArgumentError was a bit dicey from the start. One can argue that Union{MethodError,ArgumentError} is a better type for the error, but the new approach allows you to care only about the message itself (which seems to be what the original author of this test cared about).

@timholy timholy closed this Aug 20, 2021
@timholy timholy reopened this Aug 20, 2021
@timholy
Copy link
Member Author

timholy commented Aug 20, 2021

(clicked the wrong button, sorry)

Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Ah yes, technically this only depends on JuliaLang/julia#41888 which has already been merged. So this seems fine.

@c42f c42f merged commit f59bceb into master Aug 21, 2021
@c42f c42f deleted the teh/julia41885 branch August 21, 2021 04:45
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