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

Allow #[name] with #[getter] and #[setter] #1507

Merged
merged 1 commit into from
Mar 20, 2021
Merged

Conversation

scalexm
Copy link
Contributor

@scalexm scalexm commented Mar 18, 2021

A custom name can now be specified either via #[getter(custom_name)] or #[name = "custom_name"] like normal methods.

It's easier to have this kind of uniformity when dealing with code generation.

@davidhewitt
Copy link
Member

Thanks, this seems reasonable to me. I would actually like to ask your opinion on a further change relating to this.

I was thinking of moving the #[name] attribute under the pyo3 namespace, e.g. #[pyo3(name = "foo")]. I wanted to make this change everywhere that the #[name] attribute can currently be used (keeping the old form for compatibility for a little while).

This could compose with #[getter] in a slightly different form: #[pyo3(getter, name = "foo")]. See other examples where this is relevant e.g. #1504 (comment)

So the options would be

#[pymethods]
impl Foo {
    #[getter(custom_name)]
    fn get_foo(&self) { }

    // or

    #[pyo3(getter, name = "custom_name")]
    fn get_bar(&self) { }
}

The later one is longer, but I think much clearer what is going on.

@scalexm
Copy link
Contributor Author

scalexm commented Mar 18, 2021

Would you want to also allow multiple #[pyo3(...)] attributes like the following:

#[pyo3(getter)]
#[pyo3(name="custom_name")]
fn foo(&self) -> i32 { ... {

If yes, then that’s definitely a change I would support. This avoids clashing with other proc macros and adds uniformity with the existing #[pyo3(get, set)] on struct fields.

The ability to have the two attributes separately makes my life easier when generating code: in my case, adding a #[name] on every item through a macro.

@davidhewitt
Copy link
Member

Would you want to also allow multiple #[pyo3(...)] attributes like the following:

Yes; we allow that for #[derive(FromPyObject)] which uses the #[pyo3(...)] attributes already. (And I'm pretty sure serde allows this too, which we borrowed the idea from.)

@davidhewitt
Copy link
Member

If yes, then that’s definitely a change I would support.

Awesome! Are you willing to support the new syntax now in this PR, or you want to leave this PR as-is and I'll make a change to use the new syntax soon?

@scalexm
Copy link
Contributor Author

scalexm commented Mar 18, 2021

Well, to be honest I’m in lack of a personal computer right now so my ability to write code in my free time is severely decreased :)

@davidhewitt
Copy link
Member

👍 let's merge this PR and I'll write the follow-up!

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks!

@kngwyu
Copy link
Member

kngwyu commented Mar 20, 2021

I was thinking of moving the #[name] attribute under the pyo3 namespace, e.g. #[pyo3(name = "foo")].

I think it can be better, but first, shouldn't we summarize what is under the #[pyo3(...)] and what is not in an issue?

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.

3 participants