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

Positional-only args #1925

Merged
merged 10 commits into from Oct 19, 2021
Merged

Positional-only args #1925

merged 10 commits into from Oct 19, 2021

Conversation

aganders3
Copy link
Contributor

@aganders3 aganders3 commented Oct 17, 2021

Hello - thanks again for taking the time to label some issues for first-time contributors! Some of the code has moved around a bit since the comments in #1439 so hopefully this is on the right track implementation-wise.

I don't love how rust-fmt stretched out the tests, so I may also try to come up with some shorter names for them.


TODO

  • an entry in CHANGELOG.md

  • docs to all new functions and / or detail in the guide

  • tests for all new or changed functions

  • Rust tests (Just cargo test or make test if you need to test examples)

  • Rust lints (make clippy)

  • Rust formatting (cargo fmt)

  • Python formatting (black . --check. You can install black with pip install black)

  • Compatibility with all supported Python versions for all examples. This uses tox; you can do run it using make test_py.

@aganders3 aganders3 changed the title Positional only args Positional-only args Oct 17, 2021
Copy link
Member

@birkenfeld birkenfeld left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, just a few small things and a potential later refactoring opportunity.

tests/test_methods.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyfunction.rs Show resolved Hide resolved
pyo3-macros-backend/src/params.rs Show resolved Hide resolved
tests/test_methods.rs Show resolved Hide resolved
@davidhewitt davidhewitt linked an issue Oct 17, 2021 that may be closed by this pull request
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.

👍 nice work and quick turnaround, thank you for doing this!

As well as the review comments, it would be desirable to add some UI tests to tests/ui/invalid_macro_args.rs to check the error messages for some invalid function signatures (e.g. / after * and similar combinations which should be rejected.)

CHANGELOG.md Outdated Show resolved Hide resolved
pyo3-macros-backend/src/params.rs Show resolved Hide resolved
@aganders3
Copy link
Contributor Author

Thanks for the feedback and the good suggestions! I think it's just the UI tests remaining and hopefully I can get to that this week some time.

@aganders3
Copy link
Contributor Author

One more thing. Regarding this line in the issue template:

Compatibility with all supported Python versions for all examples. This uses tox; you can do run it using make test_py.

This didn't work for me without the following changes, but I'm not sure if it's something about my environment. I'm happy to add this to this PR or start a new one if it's something you'd like.

diff --git a/Makefile b/Makefile
index fdd116c4b..95e482453 100644
--- a/Makefile
+++ b/Makefile
@@ -4,7 +4,7 @@ test: lint test_py
 	cargo test

 test_py:
-	for example in examples/*; do tox -e py -c $$example || exit 1; done
+	for example in examples/*/; do TOX_TESTENV_PASSENV=RUSTUP_HOME tox -e py -c $$example || exit 1; done

 fmt:
 	cargo fmt --all -- --check

@davidhewitt
Copy link
Member

This didn't work for me without the following changes, but I'm not sure if it's something about my environment. I'm happy to add this to this PR or start a new one if it's something you'd like.

Ah thanks for pointing out. A new PR to fix that would be brilliant 👍

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.

This PR is absolutely great, this is a nice addition to add to the 0.15 release. Thanks for implementing!

@davidhewitt davidhewitt merged commit bf26dae into PyO3:main Oct 19, 2021
@aganders3
Copy link
Contributor Author

This PR is absolutely great, this is a nice addition to add to the 0.15 release. Thanks for implementing!

You're welcome! This was a fun way to get started. Thanks for the help.

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 positional-only arguments in #[pyfunction]
3 participants