-
Notifications
You must be signed in to change notification settings - Fork 240
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
Prefix newly added globals in Python with underscores #2109
Conversation
This name is internal to uniffi, it should be explicitly named using `library.uniffi_set_event_loop`.
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.
Looks fine except for uniffi_set_event_loop
(and maybe I just misunderstood!)
@@ -75,9 +75,6 @@ | |||
{%- for c in ci.callback_interface_definitions() %} | |||
"{{ c.name()|class_name }}", | |||
{%- endfor %} | |||
{%- if ci.has_async_fns() %} |
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.
I don't think we want this change - this function is documented as being exported by the Python bindings.
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.
It's still exported. __all__
just determines what you get when you do from library import *
. You can still get it using import library; library.uniffi_set_event_loop
or from library import uniffi_set_event_loop
.
EDIT: Docs: https://docs.python.org/3/tutorial/modules.html#importing-from-a-package.
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.
I understand that - but I'm not sure this change will meet the expectations of people reading our docs.
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.
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.
(sorry for the stream of thoughts: this function is only used in such edge-cases that I think I'd also be fine with a note in the docs flagging that the symbol must be used by importing it and "from mod import *" doesn't work)
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.
Added such a note.
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.
Thanks!
Followup of #1599.