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

pyfunction: allow required positional after option #2093

Merged
merged 1 commit into from
Jan 11, 2022

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jan 10, 2022

I was testing out the current main on a large-ish project to see how easy upgrading would be, and also what the effect of the compile time improvements I've been making have been.

Overall, it seems like since 0.15.1 the reduction in LLVM lines produced has indeed improved compile times a little; in a release build the optimizer spent about 10% less time on the project.

One thing I did notice was that upgrading to main was hard, purely because of the restriction on arguments introduced by #2041. The project in question has a lot of cases with arguments after an Option, and they're all used correctly without panics.

Therefore, this PR relaxes the restriction from #2041, and instead changes the behaviour such that if a positional required argument follows an Option, instead the Option is also treated as required (user must explicitly pass None or a value).

This option-has-implicit-None-default is not well documented anywhere, and I'd like to write an issue about this at some point to consider formalizing design of this before we hit 1.0. Maybe some time after 0.16 is out.

@davidhewitt
Copy link
Member Author

I'm going to proceed to merge this. Hopefully not controversial - it's less breaking than the previous solution in #2041. We can always amend again.

@davidhewitt davidhewitt merged commit edf03c1 into PyO3:main Jan 11, 2022
@davidhewitt davidhewitt deleted the positional-option branch January 11, 2022 22:06
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.

1 participant