Skip to content

Conversation

@mkitti
Copy link
Member

@mkitti mkitti commented Apr 2, 2025

Here we employ a simplified @public macro that works for all uses in this repository.

The effectiveness of the public macro is confirmed in testing.

@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.64%. Comparing base (3655020) to head (ebec769).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #32   +/-   ##
=======================================
  Coverage   91.64%   91.64%           
=======================================
  Files          28       28           
  Lines        1185     1185           
=======================================
  Hits         1086     1086           
  Misses         99       99           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nhz2
Copy link
Member

nhz2 commented Apr 2, 2025

The recommended way is to do VERSION >= v"1.11.0-DEV.469" && eval(Meta.parse("public a, b, c"))
https://docs.julialang.org/en/v1.13-dev/manual/modules/#Export-lists

Also, ideally, the other packages don't need to import new things from Core for this.

@codecov
Copy link

codecov bot commented Apr 3, 2025

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@mkitti
Copy link
Member Author

mkitti commented Apr 3, 2025

I'm aware of that but it does it is not very easy to use, read, or maintain. Meta.parse also only works on a single line of code, so we might need to repeat it for every single line of public statements that you want to make or compact things into a single line.

The way @LilithHafner phrased this in JuliaLang/julia#55097 is this was just an alternative rather than a recommendation.

While that and the more complicated macros in Public.jl and Compat.jl do handle a more complicated case of nested macro calls, you do not do that here at all.

The Expr(:public, ...) is documented here:
https://docs.julialang.org/en/v1.13-dev/devdocs/ast/#Imports-and-such

I will also note that Core.Expr is marked as public but Meta.parse is not in Julia 1.11. I think it might become public in Julia 1.12, so pick your poison here.

I do not have a clear vision of how you would want this to be implemented otherwise. My best guess is a if statement in every package that uses this and an eval(Meta.parse(...)) for every line of public usage? Maybe we should consolidate into a single public statement per package, each identifier on each line?

don't need to import new things from Core for this.

I'm unclear whether you meant Core or ChunkCodecsCore.

I have relaxed the the version bound to match so far.

Feel free to branch off or start again if you have e a clear vision of how you think it should look. Otherwise, I will try to address it once I get more feedback from you.

Copy link
Contributor

@LilithHafner LilithHafner left a comment

Choose a reason for hiding this comment

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

I recommend depending on Compat.jl rather than re-inventing the wheel here but there's nothing fundamentally wrong with this approach.

Comment on lines +2 to +9
macro public(s::Symbol)
return esc(Expr(:public, s))
end
macro public(e::Expr)
return esc(Expr(:public, e.args...))
end
else
macro public(e) end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
macro public(s::Symbol)
return esc(Expr(:public, s))
end
macro public(e::Expr)
return esc(Expr(:public, e.args...))
end
else
macro public(e) end
macro var"public"(s::Symbol)
return esc(Expr(:public, s))
end
macro var"public"(e::Expr)
return esc(Expr(:public, e.args...))
end
else
macro var"public"(e) end

Public is a keyword so it is best to protect it with var. (the same way you would have to write an @export macro).

@mkitti
Copy link
Member Author

mkitti commented Apr 3, 2025

Added another iteration on this in #33

@nhz2 nhz2 closed this Apr 3, 2025
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