arrow-julia icon indicating copy to clipboard operation
arrow-julia copied to clipboard

Change scoped enums behavior

Open jonalm opened this issue 3 years ago • 7 comments

Implement suggestions for https://github.com/apache/arrow-julia/issues/308

jonalm avatar Mar 17 '22 22:03 jonalm

I will to find time for that within a couple of days. But before I do so, should we reconsider the name of the primitive type, i.e. something other than with leading underscore? I just saw the announcement of enumx.jl https://discourse.julialang.org/t/ann-enumx-jl-improved-enums-for-julia/78096 , which has implemented a similar idea, but using T as the name of the type inside the module. E.g. for @scopedenum MyEnum::Base.UInt16 A=0 B=1 the values would be accessed as MyEnum.A and the primitive type could be accessed as MyEnum.T.

jonalm avatar Mar 18 '22 22:03 jonalm

On second thought that T idea brakes down (without careful handleing) for enums containing T fields, which I guess are way more likely than _<enumname>.

edit: I changed it to inner type name to __TYPE__ , as it is equal across different enums (which are identified by module name), and not likely to be an enum value name.

jonalm avatar Mar 19 '22 22:03 jonalm

Codecov Report

Merging #309 (f5d485f) into main (4407c70) will not change coverage. The diff coverage is 79.61%.

@@           Coverage Diff           @@
##             main     #309   +/-   ##
=======================================
  Coverage   87.15%   87.15%           
=======================================
  Files          26       26           
  Lines        3301     3301           
=======================================
  Hits         2877     2877           
  Misses        424      424           
Impacted Files Coverage Δ
src/metadata/File.jl 30.61% <33.33%> (ø)
src/eltypes.jl 86.55% <60.71%> (ø)
src/metadata/Message.jl 77.68% <62.50%> (ø)
src/FlatBuffers/FlatBuffers.jl 73.17% <75.00%> (ø)
src/metadata/Schema.jl 84.35% <91.66%> (ø)
src/arraytypes/arraytypes.jl 89.52% <100.00%> (ø)
src/arraytypes/bool.jl 88.70% <100.00%> (ø)
src/arraytypes/compressed.jl 100.00% <100.00%> (ø)
src/arraytypes/dictencoding.jl 75.58% <100.00%> (ø)
src/arraytypes/fixedsizelist.jl 84.53% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4407c70...f5d485f. Read the comment docs.

codecov-commenter avatar Mar 23 '22 22:03 codecov-commenter

@quinnj Took me some time to update all usages of the enums in the package, but I got all tests to pass now.

jonalm avatar Mar 23 '22 22:03 jonalm

I went ahead and rebased

quinnj avatar Apr 09 '22 23:04 quinnj

@quinnj Is there something blocking getting this into main?

jonalm avatar Apr 20 '22 08:04 jonalm

Would it be possible to replace the custom implementation of scoped enums here with a dependency on EnumX? That has no dependencies and is very small.

ararslan avatar Sep 22 '22 20:09 ararslan

Sorry all that this languished; I've since become more familiar w/ the EnumX.jl package and agree it's the right way to go. I'll try to do a PR soon switching over to use it. Sorry @jonalm for all the headache here, but thanks for all the help along the way!

quinnj avatar May 24 '23 04:05 quinnj