Skip to content

Allow #[classattr] on associated constants#914

Merged
kngwyu merged 2 commits intoPyO3:masterfrom
scalexm:const
May 9, 2020
Merged

Allow #[classattr] on associated constants#914
kngwyu merged 2 commits intoPyO3:masterfrom
scalexm:const

Conversation

@scalexm
Copy link
Contributor

@scalexm scalexm commented May 8, 2020

#[pymethods]
impl Foo {
    #[classattr]
    const MY_CONST: &'static str = "foobar";
}

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Very pleased to see this 👍

pub fn impl_py_class_attribute(spec: &FnSpec<'_>, wrapper: &TokenStream) -> TokenStream {
let python_name = &spec.python_name;
Copy link
Member

Choose a reason for hiding this comment

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

I understand why you did this refactoring but I think I preferred how it was before, to keep symmetry with the the other impl_py_method_xyz functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hesitated making this change indeed. I’ll write two functions, it’ll duplicate code slightly but will preserve the symmetry.

@davidhewitt
Copy link
Member

Also I think the original #905 missed off a CHANGELOG entry, so can you add one here?

(Might want to wait to rebase on #915 to make life easier.)

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.

💯

@kngwyu
Copy link
Member

kngwyu commented May 9, 2020

Thank you!

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