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

sort for NTuple and other iterables #804

Merged
merged 4 commits into from
Aug 2, 2023

Conversation

mtfishman
Copy link
Contributor

@mtfishman mtfishman commented Jul 31, 2023

Closes #803.

@mtfishman mtfishman changed the title sort for NTuple and iterables sort for NTuple and other iterables Jul 31, 2023
@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Merging #804 (dc7d33a) into master (cc4763c) will decrease coverage by 0.12%.
The diff coverage is 92.30%.

❗ Current head dc7d33a differs from pull request most recent head 38ed62b. Consider uploading reports for the commit 38ed62b to get more accurate results

@@            Coverage Diff             @@
##           master     #804      +/-   ##
==========================================
- Coverage   92.54%   92.42%   -0.12%     
==========================================
  Files           2        2              
  Lines         295      317      +22     
==========================================
+ Hits          273      293      +20     
- Misses         22       24       +2     
Files Changed Coverage Δ
src/Compat.jl 92.90% <92.30%> (-0.17%) ⬇️

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 except for some minor nit regarding import usage.

src/Compat.jl Outdated Show resolved Hide resolved
@mtfishman
Copy link
Contributor Author

mtfishman commented Aug 1, 2023

I improved the use of import, and switched over to just using using and overloading with Base.sort.

I also switched over to that style for code I introduced in #799 for overloading round/trunc/ceil/floor, hope that's ok to do in this PR.

Finally, I realized I dropped the definition:

sort(v::AbstractVector; kws...) = sort!(copymutable(v); kws...)

which was included in JuliaLang/julia#46104. It doesn't seem to be totally necessary since it is essentially the same as the more general sort(v), but skips some checks that aren't needed for AbstractVector subtypes. Seems best to just directly have the same code as Base. EDIT: Removed in the latest version since it is already in Base for older versions of Julia.

src/Compat.jl Outdated Show resolved Hide resolved
Co-authored-by: Martin Holters <[email protected]>
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 now, thanks.

@ericphanson
Copy link

This should probably be reverted as it was reverted upstream: JuliaLang/julia#52010

@martinholters
Copy link
Member

We probably need to deprecate this here instead of reverting completely.

@martinholters
Copy link
Member

#811

martinholters added a commit that referenced this pull request Nov 29, 2023
martinholters added a commit that referenced this pull request Dec 1, 2023
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.

sort for NTuple and other iterables
3 participants