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

Change scoped enums behavior #309

Closed
wants to merge 9 commits into from
Closed

Conversation

jonalm
Copy link
Contributor

@jonalm jonalm commented Mar 17, 2022

Implement suggestions for #308

Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I like this approach; the failing tests are probably because we need to update the usages in the Arrow package itself

@jonalm
Copy link
Contributor Author

jonalm commented Mar 18, 2022

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
Copy link
Contributor Author

jonalm commented Mar 19, 2022

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.

@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2022

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.

@jonalm
Copy link
Contributor Author

jonalm commented Mar 23, 2022

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

@jonalm jonalm changed the title WIP change scoped enums behavior Change scoped enums behavior Mar 25, 2022
@jonalm jonalm requested a review from quinnj April 5, 2022 13:22
quinnj
quinnj previously approved these changes Apr 9, 2022
Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this is a good correction; thanks @jonalm. The T.__TYPE__ isn't ideal, but I think it's a decent solution.

@quinnj
Copy link
Member

quinnj commented Apr 9, 2022

I went ahead and rebased

@jonalm
Copy link
Contributor Author

jonalm commented Apr 20, 2022

@quinnj Is there something blocking getting this into main?

@ararslan
Copy link

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.

@quinnj
Copy link
Member

quinnj commented May 24, 2023

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 quinnj closed this May 24, 2023
quinnj added a commit that referenced this pull request May 24, 2023
quinnj added a commit that referenced this pull request May 25, 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.

4 participants