Proposal: change @scopedenum to make modules to avoid type piracy
#267
Conversation
Example: - `Meta.UnionMode.Sparse` => `Meta.UnionModes.Sparse`
```julia julia> Arrow.FlatBuffers.@scopedenum MyEnum X=1 Y=2 Main.MyEnums ```
|
Verifying this fixes #267: julia> using BenchmarkTools
julia> @btime sort!([rand([Int,Float64,Int8,Int32]) for _ in 1:100], by=fieldcount);
59.247 μs (103 allocations: 12.33 KiB)
julia> using Arrow
[ Info: Precompiling Arrow [69666777-d1a9-59fb-9406-91d4454c9d45]
julia> @btime sort!([rand([Int,Float64,Int8,Int32]) for _ in 1:100], by=fieldcount);
53.731 μs (103 allocations: 12.33 KiB) |
|
:o oh shoot, thanks @ericphanson! Very interesting. Makes sense. Thanks. So, should we wait until all that is done? Do you know how long that will take? In the meantime, I think we'll do our prototyping against my fork from which i made this PR, which should keep us unblocked for now i guess...? |
|
Maybe @quinnj knows about the timeline? I don't know myself, sorry. Developing against the fork makes sense to me. As an alternative, if it will be awhile, I have an idea that I have never tried but I think might work. If you have a private registry you could register your fork there (with the same name + uuid). The way the package manager works is it will take versions from any installed registry. So you could do something like register patches like |
|
Hah, yeah cool. For better or worse we just have one big monorepo with only one manifest, so it's not a problem for us to build against the fork, but thanks, that's a super interesting idea if we ever changed to that approach!! Hope you've been doing well!! |
|
It shouldn't be too long; I think we're just waiting on @jrevels to sign the CLA and then we can make the transfer. |
|
Cool, thanks Jacob! :) Lemme know if you want to chat about this at all when you're ready. 👍 |
|
xref #265 (comment), hopefully we're one step closer to unblocking this :) happy to see that Relational is also making use of Arrow.jl! |
There was a problem hiding this comment.
Hmmm......yeah, I guess this is ok. I don't love the magical "text transformation" to make it plural, but I'm not sure there are many other better ideas. I had wanted to eventually split this out into a separate package or something, but perhaps we should just make a push to put in Base and be able to resolve the performance problems there perhaps.
|
@kou, can you approve the tests running here and if passing, merge please? |
|
Ah yes, I forgot those aren't enabled yet. Ok, we'll wait. |
One better text transformation which would be more reliable than the pluralization could be to rename But thanks for the approval, yeah, i didn't think of any better alternatives either. 👍 |
|
CI is runnable again. |
Codecov Report
@@ Coverage Diff @@
## main #267 +/- ##
=======================================
Coverage ? 86.97%
=======================================
Files ? 26
Lines ? 3262
Branches ? 0
=======================================
Hits ? 2837
Misses ? 425
Partials ? 0 Continue to review full report at Codecov.
|
|
Hooray. Thanks! How will the release process work under the new setup? Should I have bumped a version number in my PR? Or will you do that separately afterwards? |
This changes the result of
@scopedenumto produce a module instead of trying to stick the enums into the enum type, as a proposed fix for #266.It's admittedly less pretty, but I think it's still pretty nice! It seems like a hopefully somewhat understandable pattern to follow, which is that it pluralizes your enum name to make a module. We communicate that to the users by returning it at the end of the expression:
I can see why this is less appealing than what was there before, but what is there before suffers from the performance problems identified in #266.
Thanks for your consideration! ❤️