-
Notifications
You must be signed in to change notification settings - Fork 763
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
Fix missing feature flags in implementation of Either conversion. #3722
Conversation
Since we'd need to do a 0.20.2 release to fix our doc builds, we might want to backport #3721 as well. |
Another thing we want to add here is a CI step (at least for full builds) matching docs.rs, i.e. using a nightly rustdoc and the features and cfgs defined via |
c1c25ad
to
2e79c55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this, I agree we should do a patch release to fixup docs, and a CI job is a very good idea.
.github/workflows/ci.yml
Outdated
- uses: dtolnay/rust-toolchain@nightly | ||
with: | ||
components: rust-src | ||
- run: cargo rustdoc --lib --no-default-features --features "macros num-bigint num-complex hashbrown serde multiple-pymethods indexmap eyre either chrono rust_decimal" -Zunstable-options --config "build.rustdocflags=[\"--cfg\", \"docsrs\"]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the difference between this and full
is that it doesn't include experimental-inspect
but does include multiple-pymethods
.
To help keep things consistent should we take experimental-inspect
out of full
(and manually add it back in to relevant CI jobs) and then make this
- run: cargo rustdoc --lib --no-default-features --features "macros num-bigint num-complex hashbrown serde multiple-pymethods indexmap eyre either chrono rust_decimal" -Zunstable-options --config "build.rustdocflags=[\"--cfg\", \"docsrs\"]" | |
- run: cargo rustdoc --lib --no-default-features --features full -Zunstable-options --config "build.rustdocflags=[\"--cfg\", \"docsrs\"]" |
(Or --features "full multiple-pymethods"
, but I think multiple-pymethods
has no documentation differences, just internal machinery.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Or maybe we should be documenting experimental-inspect
also; it might help people notice the feature and motivate someone to come along and help finish it off.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think documenting experimental-inspect
would be preferable. Or rather, I think the only reason to not document a feature is if it breaks the docs.rs build or we do not want people to use it, neither of which appears to be the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a commit unifying the docs.rs and full builds.
…y making it equivalent to a full build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks!
Still broken on 0.20.2: https://github.com/PyO3/pyo3/blob/main/src/conversions/either.rs#L97 |
No description provided.