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

sorting API refactor using keywords #3665

Merged
merged 7 commits into from
Jul 11, 2013
Merged

Conversation

StefanKarpinski
Copy link
Member

Quoting the main commit:

The idea here is that the ordering and algorithm for sorting are two orthogonal parameters that have sensible defaults: neither, both or either can be independently specified. The resulting API is slightly more verbose but also somewhat more self-documenting.

Another sign that this might be on the right track is that this change reduces the LOC of base/sort.jl by 41 lines and reduces the method count of sort, for example, from 12 to 6.

Still needs deprecation methods before merging, but I thought I'd put it out there for feedback.

Note: I really wish that the lt and by ordering variations could be cleanly specified this way too – that would really cut the API down to a nice lean size without sacrificing any functionality. But I can't figure out a good way to do it.

@StefanKarpinski
Copy link
Member Author

Ok, I took a crack at using keywords for by and lt as well and I really rather like the resulting API. It eliminates the need for sortby! and sortby – and generalizes naturally to other order-related functions without blowing up the export set or requiring the use of awkward types that really should be implementation details of the sorting code.

It has, however, revealed what seems to be a method dispatch bug. Will file an issue.

The idea here is that the ordering and algorithm for sorting are
two orthogonal parameters that have sensible defaults: neither,
both or either can be independently specified. The resulting API
is slightly more verbose but also somewhat more self-documenting.

Another sign that this might be on the right track is that this
change reduces the LOC of base/sort.jl by 41 lines and reduces the
method count of sort, for example, from 12 to 6.
Also added lt, by, order, rev keywords to the searchsorted* funcs.
StefanKarpinski added a commit that referenced this pull request Jul 11, 2013
@StefanKarpinski StefanKarpinski merged commit f951d7c into master Jul 11, 2013
@StefanKarpinski StefanKarpinski deleted the sk/sort-keywords branch July 11, 2013 21:52
@stevengj
Copy link
Member

@StefanKarpinski gets the hornéd hat of shame for checking this in without updating the documentation...

@ViralBShah
Copy link
Member

I hate to do this.

+1

@StefanKarpinski
Copy link
Member Author

I'm always wearing a horned hat of shame.

@GunnarFarneback
Copy link
Contributor

This ought to give a deprecation message, I suppose:

julia> sort(>, [1:5])
ERROR: no method sort(Function,Array{Int64,1})

@StefanKarpinski
Copy link
Member Author

Yes, thanks – I missed that one.

KristofferC pushed a commit that referenced this pull request Oct 17, 2023
Stdlib: Pkg
URL: https://github.com/JuliaLang/Pkg.jl.git
Stdlib branch: master
Julia branch: master
Old commit: b02fb9597
New commit: ffb6edf03
Julia version: 1.11.0-DEV
Pkg version: 1.11.0
Bump invoked by: @IanButterworth
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Pkg.jl@b02fb95...ffb6edf

```
$ git log --oneline b02fb9597..ffb6edf03
ffb6edf03 cache pidlock tweaks (#3654)
550eadd7e Pin registry for MetaGraph tests (#3666)
ee39026b8 Remove test that depends on Random being in the sysimg (#3656)
561508db2 CI: Increase the CI timeout. Update actions. Fix double precompilation. (#3665)
7c7ed63b1 Remove change UUID script it should be uncessary on Julia v1.11-dev (#3655)
a8648f7c8 Precompile: Fix algorithmic complexity of cycle detection (#3651)
0e0cf4514 Switch datastructure Vector -> Set for algorithmic complexity (#3652)
894cc3f78 respect if load-time precompile is disabled (#3648)
3ffd1cf73 Make auto GC message use printpkgstyle (#3633)
```

Co-authored-by: Dilum Aluthge <dilum@aluthge.com>
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.

4 participants