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

Remove usage of Base internals (permute!!) #87

Merged
merged 3 commits into from
Sep 17, 2023

Conversation

LilithHafner
Copy link
Contributor

@LilithHafner LilithHafner commented Sep 10, 2023

permute!! and invpermute!! are internal functions of Base and are likely to be removed soon. This PR makes it so that if they are gone, PooledArrays will still compile.

AFAICT, nobody uses these methods anyway, so deleting them would work too, but this approach has a more minimal impact.

@bkamins
Copy link
Member

bkamins commented Sep 10, 2023

I would just remove them.

@nalimilan, @quinnj - what do you think?

@codecov
Copy link

codecov bot commented Sep 10, 2023

Codecov Report

Patch coverage has no change and project coverage change: +1.57% 🎉

Comparison is base (1bb9c95) 87.97% compared to head (762875c) 89.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
+ Coverage   87.97%   89.55%   +1.57%     
==========================================
  Files           1        1              
  Lines         341      335       -6     
==========================================
  Hits          300      300              
+ Misses         41       35       -6     
Files Changed Coverage Δ
src/PooledArrays.jl 89.55% <ø> (+1.57%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nalimilan
Copy link
Member

If they are removed from Base I agree we can just drop them.

@LilithHafner LilithHafner changed the title Guard usage of Base internals (permute!!) Remove usage of Base internals (permute!!) Sep 11, 2023
@nalimilan
Copy link
Member

Thanks. Let's just wait for the decision in Julia then.

@LilithHafner
Copy link
Contributor Author

I was holding off on merging the Base PR until after this to avoid transient breakage. If you are not willing to delete these methods until the Base PR merges, then we can revert this PR to 94beed6 which should be pretty uncontroversial. FWIW, I am not aware of any packages that call the methods this PR removes.

@nalimilan
Copy link
Member

OK, I thought that the PR was still being discussed in Julia. If all that's blocking it is PRs in packages let's merge this.

@LilithHafner
Copy link
Contributor Author

bump

@bkamins
Copy link
Member

bkamins commented Sep 17, 2023

@LilithHafner - do you need a release. If yes - can you please bump the version of the package to 1.4.3 in this PR and then I can merge it and make a release.

@LilithHafner
Copy link
Contributor Author

Yes, I do need a release, thank you.

@bkamins bkamins merged commit 25d6928 into JuliaData:main Sep 17, 2023
6 checks passed
@LilithHafner LilithHafner deleted the patch-1 branch September 17, 2023 20:21
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