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

Add the @public macro #1

Merged
merged 2 commits into from
Jan 20, 2024
Merged

Add the @public macro #1

merged 2 commits into from
Jan 20, 2024

Conversation

DilumAluthge
Copy link
Member

No description provided.

src/Public.jl Outdated
end

# export public
# @public @public
Copy link

Choose a reason for hiding this comment

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

export already marks members as public, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, so we only need one of either export @public or @public @public.

For now, I went with export @public.

@DilumAluthge DilumAluthge marked this pull request as ready for review January 20, 2024 06:35
@DilumAluthge
Copy link
Member Author

The basic functionality is now done. Let's merge this, and then I'll set up CI in a follow-up PR.

@DilumAluthge DilumAluthge merged commit 0661d0a into main Jan 20, 2024
@DilumAluthge DilumAluthge deleted the dpa/public-macro branch January 20, 2024 06:37
@KristofferC
Copy link
Member

KristofferC commented Jan 20, 2024

Isn't a whole package for a new feature a bit "excessive"? And if we wanted this to be a macro couldn't the original feature just have been a macro?

If you want backwards compat I would assume you would just put an if for the version and then have an eval + a Meta.parse like you do to support all the other new syntaxes.

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Jan 20, 2024

It's kind of annoying to have to do eval+parse in package code.

Ideally, in package code, I could just do this:

@static if VERSION >= v"1.11-"
    public foo, bar
end

But that doesn't work.

And if we wanted this to be a macro couldn't the original feature just have been a macro?

Actually, yeah if we could make the original feature (in Base) a macro, then I could do this in package code:

@static if VERSION >= v"1.11-"
    Base.@public foo, bar
end

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Jan 20, 2024

I think that's my preferred solution here. In Julia 1.11, add a macro to Base of the form Base.@public such that @public foo, bar is equivalent to public foo, bar. And then package code just does:

@static if VERSION >= v"1.11-"
    Base.@public foo, bar
end

And then there's no need for this package (Public.jl), and package authors don't also need to take a dependency on Compat.jl.

@goerz
Copy link

goerz commented Jan 20, 2024

Can that work without the Base. prefix in front of @public?

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Jan 20, 2024

Sure, we can export @public from Base, and then in package code you'd just do:

@static if VERSION >= v"1.11-"
    @public foo, bar
end

@goerz
Copy link

goerz commented Jan 20, 2024

That seems quite appealing! I’d definitely use that

@KristofferC
Copy link
Member

It's kind of annoying to have to do eval+parse in package code

Yeah a bit but adding a whole package (that will end up in almost every manifest for a very long time) is also kind of annoying.

A macro in Base or adding to Compat.jl instead would be preferable in my opinion.

@fredrikekre
Copy link
Member

This feature already exist in Compat.jl (which I thought was the JuliaLang solution to compatibility problems). Doesn't really seem sustainable to maintain a package like this per new feature.

@LilithHafner
Copy link
Member

In 3-8 years, or whenever folks start dropping compat for 1.10, I prefer

export baz
public foo, bar

over

export baz
@public foo, bar

In the interem, I think

using Compat
export baz
@compat public foo, bar

is fine; the point of Compat is to allow Base to add new features and let folks get those features while keeping compat from before they were implemented, and I think it does a good enough job.

Today, there may be a large number of packages which only want Compat.jl for the public keyword, so it is tempting to excise that functionality into another package and write

using Public
export baz
@public foo, bar

But as time passes and the public keyword stops being the most recent major Compat.jl addition, it will be increasingly common for folks who need @public to also need other compat features.

FWIW, on my computer, Compat.jl takes 8 seconds to install as a standalone package (mostly downloading the general registry), 42ms to load the first time, and, after restarting Julia, 3.6ms to load. It is also already in most large manifests.

Doesn't really seem sustainable to maintain a package like this per new feature.

In theory, a micro package could have a near-zero maintenance burden. If would be nice if that were actually the case.

@KristofferC
Copy link
Member

KristofferC commented Jan 20, 2024

Is writing eval(Meta.parse really that much worse than adding a whole dependency to your package...?

Anyway, whatever solution is good that doesn't cause this type of one package per feature thing to happen. If that is a macro in Base or in Compat or whatever doesn't matter much to me.

@goerz
Copy link

goerz commented Jan 20, 2024

Is writing eval(Meta.parse really that much worse

How does that look, exactly? Something like this?

@static if VERSION >= v"1.11-"
eval(Meta.parse, quote
    public foo, bar
end)
end

@KristofferC
Copy link
Member

KristofferC commented Jan 20, 2024

eval(Meta.parse("..."))

For example https://github.com/JuliaLang/Tokenize.jl/blob/13396b85c2847544e9231cebb509de5c5674e46b/src/token_kinds.jl#L1497

@LilithHafner
Copy link
Member

LilithHafner commented Jan 20, 2024

You can also do

VERSION >= v"1.11.0-DEV.469" && eval(Expr(:public,
    :foo,
    :bar
))

if you want to drop that pesky dependency on Meta.parse. 😄

https://github.com/JuliaLang/julia/blob/bdd3ffdd215db8233a43b6ba34c709add2e26b3c/base/exports.jl#L1068-L1073

Edit to clarify: unless you are doing some extreme tree-shaking for binary size, or have some other reason to prefer this approach, it's not a great idea. "...that pesky dependency on Meta.parse" is a joke, though in bootstrapping can be less of a joke.

@KristofferC
Copy link
Member

Seems worse to rely on some (undocumented?) Expr head..

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.

5 participants