Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for #[new] which is also a #[classmethod] #3157

Merged
merged 1 commit into from
May 17, 2023

Conversation

stuhood
Copy link
Contributor

@stuhood stuhood commented May 16, 2023

Fixes #3077.

@stuhood stuhood force-pushed the stuhood/classmethod-new branch 3 times, most recently from 329ef94 to 756d150 Compare May 16, 2023 19:29
@DataTriny
Copy link
Contributor

DataTriny commented May 16, 2023

First time contributor has agreed to the new licensing scheme.

@stuhood stuhood force-pushed the stuhood/classmethod-new branch from 756d150 to 716de0b Compare May 16, 2023 20:09
@stuhood stuhood marked this pull request as ready for review May 16, 2023 20:10
@stuhood stuhood force-pushed the stuhood/classmethod-new branch from 716de0b to 6c3fd07 Compare May 16, 2023 20:22
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.

Thanks! The implementation is fine; I'm not a huge fan of a new enum variant in FnType though I can't see an obviously better way to do it. I think better to deliver the working feature and then hopefully over time refactoring will present an option.

Some suggestions for docs & tests.

@stuhood stuhood force-pushed the stuhood/classmethod-new branch from 8818b2f to 55a217f Compare May 16, 2023 22:27
@stuhood
Copy link
Contributor Author

stuhood commented May 16, 2023

Thanks! Have applied that feedback.

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.

Looks great to me, thanks! I'll leave for a moment in case anyone else wants to review. If you're willing to squash that would also be appreciated; we like to keep the git history neat when we can 😊

@stuhood stuhood force-pushed the stuhood/classmethod-new branch from 55a217f to 20c5618 Compare May 16, 2023 22:51
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.

Thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented May 17, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 3b4c7d3 into PyO3:main May 17, 2023
@stuhood stuhood deleted the stuhood/classmethod-new branch May 18, 2023 03:34
bors bot added a commit that referenced this pull request May 29, 2023
3188: Verify that macros do work without any imports r=adamreichold a=lifthrasiir

Currently virtually all (positive) tests import `pyo3::prelude::*`, making it hard to detect a certain class of bugs. This PR adds an explicit test that never imports from `pyo3` to fix this.

Also this fixes a minor bug from #3157 which didn't work without `use pyo3::types::PyType;`. I think this should be a part of 0.19.0 (#3187), so no additional changelog would be required (as this feature is new in 0.19.0).


Co-authored-by: Kang Seonghoon <public+git@mearie.org>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[new] which is also a #[classmethod]
4 participants