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

support text_signature on #[new] #2980

Merged
merged 2 commits into from
May 4, 2023
Merged

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Feb 22, 2023

Closes #2866

This is a breaking change for 0.19.0, because it starts autogenerating text_signature for #[new]. This could affect runtime behaviour if the user is relying on the class docs at runtime for some reason.

Guide & tests all updated accordingly.

#[pyclass(text_signature = "...")] is deprecated by this PR, however if it's set, it will be used in preference to #[new].

(The signature / text_signature from #[new] will simply be ignored in this case. I figure that when users fix their deprecation warnings by removing #[pyclass(text_signature = "...")] then the #[new] signatures will start flowing properly, and this is good enough.)

@davidhewitt davidhewitt force-pushed the text-signature-new branch 2 times, most recently from 92a88fa to 3c9fbe7 Compare April 18, 2023 20:03
@davidhewitt
Copy link
Member Author

@adamreichold I've just rebased this, and I think (assuming CI passes) it might be ready to merge.

Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

Single nit. Please feel free to merge after resolving this.

@davidhewitt
Copy link
Member Author

bors r=adamreichold

bors bot added a commit that referenced this pull request Apr 19, 2023
2980: support `text_signature` on `#[new]` r=adamreichold a=davidhewitt

Closes #2866 

This is a breaking change for 0.19.0, because it starts autogenerating `text_signature` for `#[new]`. This could affect runtime behaviour if the user is relying on the class docs at runtime for some reason.

Guide & tests all updated accordingly.

`#[pyclass(text_signature = "...")]` is deprecated by this PR, however if it's set, it will be used in preference to `#[new]`.

(The signature / `text_signature` from `#[new]` will simply be ignored in this case. I figure that when users fix their deprecation warnings by removing `#[pyclass(text_signature = "...")]` then the `#[new]` signatures will start flowing properly, and this is good enough.)

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Apr 19, 2023

Build failed:

@adamreichold
Copy link
Member

bors retry

bors bot added a commit that referenced this pull request Apr 20, 2023
2980: support `text_signature` on `#[new]` r=adamreichold a=davidhewitt

Closes #2866 

This is a breaking change for 0.19.0, because it starts autogenerating `text_signature` for `#[new]`. This could affect runtime behaviour if the user is relying on the class docs at runtime for some reason.

Guide & tests all updated accordingly.

`#[pyclass(text_signature = "...")]` is deprecated by this PR, however if it's set, it will be used in preference to `#[new]`.

(The signature / `text_signature` from `#[new]` will simply be ignored in this case. I figure that when users fix their deprecation warnings by removing `#[pyclass(text_signature = "...")]` then the `#[new]` signatures will start flowing properly, and this is good enough.)

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented Apr 20, 2023

Build failed:

@adamreichold
Copy link
Member

The nightly warning on non_upper_case_globals seems unrelated but also trivial to fix by adding an #[allow(..)] to the generated code so I'd say we might as well do it here?

The test failure in test_auto_test_signature_method seems real though. During review, I assumed that this would be the automatically generated Python signature based on the actual Rust signature? Does that not work on Python 3.7? (It would be very helpful to have a py_assert_eq to see the actual value in the logs.)

@adamreichold
Copy link
Member

adamreichold commented Apr 21, 2023

The nightly warning on non_upper_case_globals seems unrelated but also trivial to fix by adding an #[allow(..)] to the generated code so I'd say we might as well do it here?

Actually, I cannot reproduce this on main. Maybe this does need a rebuildase? Probably useful anyway after #3114 lands.

@davidhewitt davidhewitt force-pushed the text-signature-new branch from 8acb224 to 8bd17f0 Compare May 4, 2023 06:15
@davidhewitt
Copy link
Member Author

Rebased; let's hope things are working again now. Sorry for the gap!

bors r=adamreichold

bors bot added a commit that referenced this pull request May 4, 2023
2980: support `text_signature` on `#[new]` r=adamreichold a=davidhewitt

Closes #2866 

This is a breaking change for 0.19.0, because it starts autogenerating `text_signature` for `#[new]`. This could affect runtime behaviour if the user is relying on the class docs at runtime for some reason.

Guide & tests all updated accordingly.

`#[pyclass(text_signature = "...")]` is deprecated by this PR, however if it's set, it will be used in preference to `#[new]`.

(The signature / `text_signature` from `#[new]` will simply be ignored in this case. I figure that when users fix their deprecation warnings by removing `#[pyclass(text_signature = "...")]` then the `#[new]` signatures will start flowing properly, and this is good enough.)

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
@bors
Copy link
Contributor

bors bot commented May 4, 2023

Build failed:

@adamreichold
Copy link
Member

I think

The test failure in test_auto_test_signature_method seems real though. During review, I assumed that this would be the automatically generated Python signature based on the actual Rust signature? Does that not work on Python 3.7? (It would be very helpful to have a py_assert_eq to see the actual value in the logs.)

still applies.

@adamreichold
Copy link
Member

Sadly, I cannot reproduce the issue locally even using Python 3.7, neither via pyenv (which is 3.7.16 as in GitHub actions) nor using indygreg's standalone builds which stopped at 3.7.9.

@adamreichold
Copy link
Member

Sadly, I cannot reproduce the issue locally even using Python 3.7, neither via pyenv (which is 3.7.16 as in GitHub actions) nor using indygreg's standalone builds which stopped at 3.7.9.

Sorry, I am an idiot for trying this on the main branch...

@adamreichold
Copy link
Member

adamreichold commented May 4, 2023

I think this is due to us not being able to set __text_signature__ on classes using the limited API before version 3.10. I hence took the liberty of pushing a commit ignoring that one assertion as we already do for the class_with_signature_no_doc and class_with_docs_and_signature test cases.

bors r+

bors bot added a commit that referenced this pull request May 4, 2023
2980: support `text_signature` on `#[new]` r=adamreichold a=davidhewitt

Closes #2866 

This is a breaking change for 0.19.0, because it starts autogenerating `text_signature` for `#[new]`. This could affect runtime behaviour if the user is relying on the class docs at runtime for some reason.

Guide & tests all updated accordingly.

`#[pyclass(text_signature = "...")]` is deprecated by this PR, however if it's set, it will be used in preference to `#[new]`.

(The signature / `text_signature` from `#[new]` will simply be ignored in this case. I figure that when users fix their deprecation warnings by removing `#[pyclass(text_signature = "...")]` then the `#[new]` signatures will start flowing properly, and this is good enough.)

Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Co-authored-by: Adam Reichold <adam.reichold@t-online.de>
@bors
Copy link
Contributor

bors bot commented May 4, 2023

Build failed:

@adamreichold
Copy link
Member

bors retry

@bors
Copy link
Contributor

bors bot commented May 4, 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 1cdac4f into PyO3:main May 4, 2023
@davidhewitt davidhewitt deleted the text-signature-new branch May 4, 2023 17:38
@davidhewitt
Copy link
Member Author

Thanks 😅

bors bot added a commit that referenced this pull request May 9, 2023
2981: Remove 0.17 deprecations r=adamreichold,davidhewitt a=davidhewitt

Since #2980 starts a breaking change for 0.19, let's also clean up all 0.17's deprecations.

I've removed `Python::acquire_gil` in its own commit, as that was a reasonably chunky removal.

Co-authored-by: Adam Reichold <adam.reichold@t-online.de>
Co-authored-by: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
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.

Support text_signature and autogenerated text signature for #[new]
2 participants