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.

Sign up for GitHub

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

Jump to bottom

Allow type as argument to throwerrors #2

Merged
merged 2 commits into from
Oct 29, 2020
Merged

Allow type as argument to throwerrors #2

merged 2 commits into from
Oct 29, 2020

Conversation

MichaelHatherly
Copy link
Member

@MichaelHatherly MichaelHatherly commented Oct 26, 2020 •
edited
Loading

Comments and docstrings probably need adjusting, I'll do that this afternoon if I have time.

MichaelHatherly
MichaelHatherly commented Oct 26, 2020
View reviewed changes
test/runtests.jl Outdated
# Invalid throwerrors values
@test_throws DomainError iocapture(()->nothing, throwerrors=:foo)
@test_throws TypeError iocapture(()->nothing, throwerrors=42)
@test_throws DomainError iocapture(()->nothing, throwerrors=42)
Copy link
Member Author

Choose a reason for hiding this comment

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

Are we bothered about this change at all?

Copy link
Member

Choose a reason for hiding this comment

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

I would keep the test, but I wouldn't consider the exception type to be part of the public API, if that's what you mean.

MichaelHatherly
MichaelHatherly commented Oct 26, 2020
View reviewed changes
src/IOCapture.jl Outdated
# - `true` -> `Any`,
# - `false` -> `Union{}`,
# - `:interrupt` -> `InterruptException`.
throwerrors = rewrite_error_argument(throwerrors)
Copy link
Member Author

Choose a reason for hiding this comment

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

Ain't terribly type-stable, but considering what else usually gets called in iocapture I doubt it's of any concern.

@MichaelHatherly
Copy link
Member Author

Worth deprecating true, false, and :interrupt or shall we just leave them as is? I'm in favour of just leaving them in since it doesn't really make things any more complicated.

@mortenpi
Copy link
Member

I would just get rid of true/false/:interrupt since they are duplicating the ::Type-based interface, unless you think it would be useful for the user if we keep them? It's fine that it would be a breaking change -- I intentionally tagged this as 0.1, so that it would be possible to iterate on the design a little if need be.

@mortenpi mortenpi added the enhancement New feature or request label Oct 26, 2020
@mortenpi mortenpi mentioned this pull request Oct 26, 2020
Closed
mortenpi
mortenpi reviewed Oct 26, 2020
View reviewed changes
src/IOCapture.jl Outdated
Comment on lines 24 to 27
Setting it to `Union{}` will capture all thrown exceptions and return them
via the `.value` field, and will also set `.error` and `.backtrace`
accordingly. Setting it to `false` also has this effect.

To throw on a subset of possible exceptions pass the exception type instead,
such as `InterruptException`. If multiple exception types may need to be
thrown then pass a `Union{...}` of the types. Passing `:interrupt` has the
same effect as using `InterruptException`.
Copy link
Member

Choose a reason for hiding this comment

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

I would emphasize here that the InterruptException case is the one you usually want, e.g. by changing the order of the paragraphs.

@MichaelHatherly
Copy link
Member Author

I would just get rid of true/false/:interrupt since they are duplicating the ::Type-based interface, unless you think it would be useful for the user if we keep them? It's fine that it would be a breaking change -- I intentionally tagged this as 0.1, so that it would be possible to iterate on the design a little if need be.

Either way is fine with me, shall I just drop them in this PR?

@mortenpi
Copy link
Member

shall I just drop them in this PR?

If you don't mind then yeah 🙂

@MichaelHatherly
Copy link
Member Author

Also makes default Exception rather than Any.

@mortenpi
Copy link
Member

Also makes default Exception rather than Any.

I would argue against that, because while you probably shouldn't, you can throw objects that are not subtypes of Exception:

julia> struct Foo end

julia> try
           throw(Foo())
       catch e
           @show e isa Exception
       end
e isa Exception = false
false

@MichaelHatherly
Copy link
Member Author

Not bothered either way :) Bit weird that it throws anything though, never come across that in any real code.

@mortenpi mortenpi added this to the 0.2.0 milestone Oct 29, 2020
@mortenpi
Copy link
Member

LGTM, thanks a lot!

@mortenpi mortenpi merged commit c5427a9 into master Oct 29, 2020
@mortenpi mortenpi deleted the mh/throw-types branch October 29, 2020 21:28
@mortenpi mortenpi mentioned this pull request Mar 18, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mortenpi mortenpi mortenpi left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@MichaelHatherly @mortenpi