-
Notifications
You must be signed in to change notification settings - Fork 782
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
macros: support #[pyo3(name = "...")]
in pyfunction
#1567
Conversation
adaa60e
to
d614a5b
Compare
... I'm beginning to consider that this PR could first be merged without deprecating the existing attribute syntax. We can get all the improvements which we like completed, and then finalise the process with documentation and deprecate the current syntax. |
3a4e8d0
to
04919be
Compare
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! Almost good, but isn't it OK to use lots of codes for deprecation warnings? I'm not sure this is the best way
04919be
to
1e7a1c0
Compare
Thanks for the review! I pushed a commit to address your feedback. So I assume that you overall are happy with the design proposal here? i.e. the new form of
Can you clarify what you mean by this? |
I'm sorry but I recently haven't used these macros in real so have no opinion. Hearing from users might be better.
Almost the same as you wrote
|
Yes, because with
IMO it is much better when there is only one "proper" way to do things. When figuring out a crate, what do you prefer to read:
Consider also how confusing it is to read examples where either may be used. As for simplification, I would probably prefer syntax where everything uses These two could be equivalent:
or
That would apply to everything, e.g. This is also much nicer to write docs for, because there is less repetition. |
TLDR; I'm thinking that having both forms is conveneient for now for backwards compatibility, however we should prefer Thanks for the feedback! Your
would be equivalent to today's Regardless of which form we take, this seems like a good argument to allow Similarly if we eventually want to just have If the above is what we're aiming for, then we need to consider the migration story for users from the current syntax. Over several releases we can make deprecations to gently nudge users towards what we want them to write. At the moment we have
After this first round of deprecations we might have led users to rewrite this as
I think also we would not document it as
but instead only document the
And then in a final step later we could deprecate |
That is even nicer 👍. I agree with everything you said. |
I think the consistency is an improvement, and I think that avoiding bare identifiers like There's one thing I haven't seen mentioned in this issue, though, which is the possibility of using |
I think Once we've got all these attributes consistent, if we wanted to rename from |
1e7a1c0
to
70f9d19
Compare
As all the feedback I've recieved on this has been neutral or positive, I'm going to merge this now and start making progress with the other items in the list in the OP. We can always make further changes to this ahead of 0.14 if we decide against some of the design. Thanks all for commenting! |
This PR is the first in a series of PRs which I plan to do to tidy up the macro attribute APIs, parsing, and documentation. This should make it easier for maintainers as well as users. I've spoken about this kind of idea briefly at #1507 (comment)
At the moment we have the
#[name = "..."]
attribute which can be placed on#[pyfunction]
and in#[pymethods]
. It's not documented, and it's hard to document because it's not consistent how names are set.#[pymodule]
and#[pyfn]
take a name directly as an argument e.g.#[pymodule("foo")]
and#[pyfn(m, "foo")]
, for example.As a first step I want to unify to an
#[attribute(name = "...")]
syntax, whereattribute
can be:#[pyo3(name = "foo")]
#[pyfunction(name = "foo")]
#[pymodule(name = "foo")]
etc.
#[pyo3(name = "...")]
can appear on individual#[pymethods]
, as well as alongside#[pyfunction]
. For example:Because
pyfunction_form_2
has options mixed in with the signature, I've made it possible to separate the signature out into a new (undocumented, for now) option calledsignature
:Note that mixing of
pass_module
andsignature
already exists (#1566), so adding a new explicitsignature
option helps solve the existing problem too.To review this PR:
This PR's diff is bigger than just the feature change because I needed to refactor a fair bit of the code to make this possible. At the same time this refactoring fixes #1566, and will make future PRs in this series much easier to compare.
#[name = "foo"]
to#[pyo3(name = "foo")]
? I've used a bit of macro hackery to emit deprecation warnings when the old form is used.#[pyfunction(name = "foo")]
as well as#[pyfunction] #[pyo3(name = "foo")]
?#[name]
with#[getter]
and#[setter]
#1507 (comment)).#[pyo3()]
options and say that#[pyfunction(...)]
can take all the same options as a shorthand.Future steps:
If you like the direction this is going, these are the next steps:
#[pymodule("name")]
,#[getter(name)]
and#[pyfn(m, name)]
to use the samename = "..."
form.#[text_signature = "..."]
->#[pyo3(text_signature = "...")]
/#[pyfunction(text_signature = "...")]
.signature
option topyfunction
(and implement Support positional-only arguments in #[pyfunction] #1439 now that the parsing is refactored).