-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
add public
statement to Base.Broadcast
#54060
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
base: master
Are you sure you want to change the base?
Conversation
public
statement to broadcast.jlpublic
statement to Base.Broadcast
I've removed |
@LilithHafner Any feedback on this? |
Thanks for the bump.
|
Any news? I can add a docstring for |
I was pretty surprised to find out these didn't make the first round of public declarations. Is there any reason this hasn't been pushed forward? |
I was waiting for feedback about how to proceed with |
I think namespacing it makes it less of a problem to rename it as |
What does this mean in terms of answers to my two questions? |
Sorry, I saw the triage label and justj added a quick opinion instead of answering any solid questions.
That being said, triage may have a different response. |
@@ -1179,7 +1179,7 @@ f51129(v, x) = (1 .- (v ./ x) .^ 2) | |||
@testset "Docstrings" begin | |||
undoc = Docs.undocumented_names(Broadcast) | |||
@test_broken isempty(undoc) | |||
@test undoc == [:dotview] | |||
@test undoc == [:Broadcasted, :Unknown, :dotview] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add docstrings instead of adding exceptions to this test.
Triage thinks that this is generally fine but defers to @mbauman to make sure that these are actually the public API of broadcasting. |
Should |
I'm in favor of making it public if we can agree on an interface, because I recall having to override it sometimes. For example, documenting that new method definitions should dispatch on specifically on the |
This PR makes the following symbols from
Base.Broadcast
public:I think it makes sense to make it public given that
Broadcasted
is also public.I think it makes sense to have them available as shortcuts. Maybe one should even mention them in the documentation instead of using
DefaultArrayStyle{1}
, for example.EDIT: Making
DefaultVectorStyle
andDefaultMatrixStyle
public leads to failing doctests because they now appear in the Julia output instead ofDefaultArrayStyle{1}
andDefaultArrayStyle{2}
. However, they are never used in broadcast.jl. If one doesn't want to have them public, why are they defined at all? Let me know if you want to have them public. Then I adjust the documentation.