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

Rebase no load error on macroexpand #38379

Merged
merged 8 commits into from
Apr 16, 2021
Merged

Rebase no load error on macroexpand #38379

merged 8 commits into from
Apr 16, 2021

Conversation

bramtayl
Copy link
Contributor

No description provided.

@bramtayl bramtayl mentioned this pull request Nov 11, 2020
Closed
@bramtayl
Copy link
Contributor Author

Credit for the deprecation code goes to @c42f

@c42f
Copy link
Member

c42f commented Nov 11, 2020 •
edited
Loading

XRef: this is a continuation of: #37857

Note that, generally speaking, it's better to force push your WIP branch when doing a rebase, rather than opening a new PR. This makes it easier for reviewers.

Credit for the deprecation code goes to @c42f

Thanks. By the way, the "git standard" way to add multiple authors is to add a Co-authored-by line to the git commit. See 9572202 as an example of how to do this.

@bramtayl
Copy link
Contributor Author

Oh, ok! Is it possible to edit the existing commit to include Co-authored-by? Clearly I don't have much git-fu.

@bramtayl
Copy link
Contributor Author

Pretty sure the failure here is unrelated (maybe there's profile deadlocking on Arch?)

@c42f
Copy link
Member

c42f commented Nov 12, 2020

Is it possible to edit the existing commit

Sure - use git commit --amend on your branch and edit the message, then git push -f to force push the edited history to your branch, which will update the PR.

@bramtayl bramtayl force-pushed the bramtayl_no_loaderror_2 branch from 6da368d to b2233b0 Compare November 12, 2020 13:59
@bramtayl
Copy link
Contributor Author

Thanks for the help! Hopefully that did it

@bramtayl
Copy link
Contributor Author

Bump?

@JeffBezanson JeffBezanson added the needs news A NEWS entry is required for this change label Nov 21, 2020
@JeffBezanson
Copy link
Member

Could use a news entry.

@bramtayl
Copy link
Contributor Author

Ok! How's that look?

c42f
c42f reviewed Nov 23, 2020
View reviewed changes
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.

Looking close, I added a couple more suggestions for backward compat.

stdlib/Test/test/runtests.jl Outdated
throw(ErrorException("Real error"))
end
macro test_macro_throw_2()
LoadError("file", 111, throw(ErrorException("Real error")))
Copy link
Member

Choose a reason for hiding this comment

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

This looks weird — it throws ErrorException so that LoadError is doing nothing. I see it's carried over from 3ee9d8f, but it was probably a mistake there too.

IIRC the intention here was to actually throw LoadError explicitly, as the occasional non-core library does that to mimic Base.

stdlib/Test/src/Test.jl Show resolved Hide resolved
stdlib/Test/test/runtests.jl Show resolved Hide resolved
@bramtayl
Copy link
Contributor Author

Ok, I've added a deprecation warning to undecorated @test_throws LoadError [@]macroexpand[1] (not 100% I did it right), removed the from_macroexpand check for decorated LoadErrors, and fixed @test_macro_throw_2 (and rebased via GitHub)

c42f
c42f reviewed Dec 23, 2020
View reviewed changes
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.

Nice, we're very close here. depwarn() is subtle to use correctly.

There's some test errors here to investigate. I'm not sure whether we still run all tests with --depwarn=error by default, but that might be the cause. In that case, these deprecation tests might need to be treated the same as test/deprecation_exec.jl (see test/misc.jl for how that is run)

stdlib/Test/src/Test.jl Outdated Show resolved Hide resolved
@bramtayl
Copy link
Contributor Author

Look at all that green!

@StefanKarpinski
Copy link
Member

Bump. @JeffBezanson?

@vtjnash vtjnash removed the needs news A NEWS entry is required for this change label Apr 16, 2021
@vtjnash vtjnash merged commit 07bf9da into JuliaLang:master Apr 16, 2021
@vtjnash
Copy link
Member

vtjnash commented Apr 16, 2021

Credit for the deprecation code goes to @c42f

My apologies to @c42f that I unintentionally removed this mention when trying to fix up the squash message

@bramtayl
Copy link
Contributor Author

Wooo!!!

simeonschaub added a commit to JuliaDiff/ChainRulesCore.jl that referenced this pull request May 2, 2021
@simeonschaub
fix tests on Julia nightly
2442a25 
adapt to JuliaLang/julia#38379
@simeonschaub simeonschaub mentioned this pull request May 2, 2021
Merged
simeonschaub added a commit to JuliaDiff/ChainRulesCore.jl that referenced this pull request May 2, 2021
@simeonschaub @oxinabox
fix tests on Julia nightly (#342)
bda9c71 
adapt to JuliaLang/julia#38379

Co-authored-by: Lyndon White <[email protected]>
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
@bramtayl @ElOceanografo
no LoadError wrapper in macroexpand errors (JuliaLang#38379)
0daa7bc
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
@bramtayl @antoine-levitt
no LoadError wrapper in macroexpand errors (JuliaLang#38379)
eefea02
@bramtayl bramtayl mentioned this pull request May 13, 2021
Merged
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
@bramtayl @johanmon
no LoadError wrapper in macroexpand errors (JuliaLang#38379)
c0f1c62
carlobaldassi added a commit to carlobaldassi/ArgParse.jl that referenced this pull request Feb 8, 2024
@carlobaldassi
Introduce ArgParseSettingsError
87e38ff 
used for all settings-related errors
fix use of LoadError in tests (JuliaLang/julia#38379)
also simplify the `defaults` macro in src/common.jl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c42f c42f c42f left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bramtayl @c42f @JeffBezanson @StefanKarpinski @vtjnash