-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Document the role of missing
in PartialQuickSort
and deprecate PartialQuickSort(::Integer)
#47297
Conversation
…artialQuickSort(::Integer)`
base/sort.jl
Outdated
PartialQuickSort(k::OrdinalRange) = PartialQuickSort(first(k), last(k)) | ||
@deprecate PartialQuickSort(k::Integer) PartialQuickSort(missing, k) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't use @deprecate
before, but it seems it should be placed into base/deprecated.jl
.
I also haven't used deprecations in base before, and I'd like approval from someone who is confident that this is acceptable w.r.t. SymVer and our promise of backward compatibility. |
As CI complains, the docs should be updated after merging your amazing stable https://github.com/JuliaLang/julia/blame/master/doc/src/base/sort.md#L143-L190 Just a few comments for
Further for
Since I'm writing, I cannot find the stabilization of More general question. Is there a plan to guarantee stability of I can try preparing some PRs, if you want. |
Thank you for your observations! Yes, sort.md needs updating.
That would be lovely! Thank you! I think the I happened to make a PR for the sort.jl problem you noted before I realized you were offering to make PRs, I'd be happy if you took as many of the rest as you'd like to :) |
This PR deprecates improper usage of PartialQuickSort that is still found in sort.jl, so tests will fail until #47191 which fixes those usages is merged. |
CI now passes (failures unrelated) and all this needs is approval from someone familiar with adding deprecations to Base (@vtjnash, perhaps) |
From #47191 (comment)