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

Port Base.current_exceptions() to older Julia versions #746

Merged
merged 3 commits into from
Aug 19, 2021

Conversation

c42f
Copy link
Member

@c42f c42f commented Jun 26, 2021

See JuliaLang/julia#29901

This is limited to julia-1.1 and above because earlier versions don't
have the necessary runtime library support (Base.catch_stack() etc).

@codecov
Copy link

codecov bot commented Jun 26, 2021

Codecov Report

Merging #746 (d5a5199) into master (ad9a668) will increase coverage by 0.28%.
The diff coverage is 88.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
+ Coverage   81.71%   81.99%   +0.28%     
==========================================
  Files           4        4              
  Lines         536      561      +25     
==========================================
+ Hits          438      460      +22     
- Misses         98      101       +3     
Impacted Files Coverage Δ
src/Compat.jl 82.02% <88.00%> (+0.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad9a668...d5a5199. Read the comment docs.

@c42f
Copy link
Member Author

c42f commented Jun 26, 2021

Failing tests on nightly are #745

@omus omus added the needs-news Needs an entry in the README label Jul 28, 2021
@martinholters
Copy link
Member

With a README entry and after a rebase, this looks good to me.

c42f added 2 commits August 16, 2021 17:33
See JuliaLang/julia#29901

This is limited to julia-1.1 and above because earlier versions don't
have the necessary runtime library support (Base.catch_stack() etc).
@c42f c42f force-pushed the cjf/current-exceptions branch from cff5be3 to 29e040b Compare August 16, 2021 21:51
@c42f
Copy link
Member Author

c42f commented Aug 16, 2021

I added the readme entry and rebased.

Also I've added julia-1.0 compatibility (in that case, the best we can do is just return the current exception as a single-element stack)

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

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

LGTM superficially, but you probably know better than me whether this does the right thing.

But maybe we should mention the limited functionality on Julia 1.0 in the README?

@martinholters martinholters removed the needs-news Needs an entry in the README label Aug 19, 2021
@c42f
Copy link
Member Author

c42f commented Aug 19, 2021

whether this does the right thing.

It should do the right thing to the extent that the right thing can be done at all in julia 1.0 (there were some nasty bugs with exceptions and backtraces going missing completely in julia-1.0 due to task switching. But Compat can't work around that.)

I figured it was better to have the API with only approximate runtime compatibility in 1.0, than to not have the API at all — it's a lot easier for users if they can at least rely on the API being present on all versions supported by Compat.

@martinholters
Copy link
Member

I don't disagree. Only question: Merge as is or mention the limitation re: Julia 1.0 in the README?

@c42f
Copy link
Member Author

c42f commented Aug 19, 2021

Yeah, it's worth mentioning the Julia-1.0 limitations in the readme. I did that.

@martinholters
Copy link
Member

Alright, there was plenty of time for others to chime in on this, bu no one did, so here we go...

@martinholters martinholters merged commit d79d0fc into JuliaLang:master Aug 19, 2021
@c42f c42f deleted the cjf/current-exceptions branch August 19, 2021 13:05
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.

3 participants