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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 2, 2024
Merged

Fix missing feature flags in implementation of Either conversion. #3722

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2564ca4
Fix missing feature flags in implementation of Either conversion.
adamreichold Jan 2, 2024
2e79c55
Add CI job to test the equivalent of a docs.rs build.
adamreichold Jan 2, 2024
9120b35
Include the experimental-inspect feature for the docs.rs build thereb…
adamreichold Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,22 @@ jobs:
RUST_BACKTRACE: 1
TRYBUILD: overwrite

docsrs:
if: ${{ contains(github.event.pull_request.labels.*.name, 'CI-build-full') || (github.event_name != 'pull_request' && github.ref != 'refs/heads/main') }}
needs: [fmt]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v4
- uses: Swatinem/rust-cache@v2
with:
key: cargo-careful
continue-on-error: true
- 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\"]"
Copy link
Member

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

Suggested change
- 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.)

Copy link
Member

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.)

Copy link
Member Author

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.

Copy link
Member Author

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.


coverage:
needs: [fmt]
name: coverage-${{ matrix.os }}
Expand Down Expand Up @@ -387,6 +403,8 @@ jobs:
run: nox -s test-emscripten

test-debug:
needs: [fmt]
if: github.ref != 'refs/heads/main'
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
Expand Down Expand Up @@ -438,6 +456,7 @@ jobs:
- build-full
- valgrind
- careful
- docsrs
- coverage
- emscripten
if: always()
Expand Down
6 changes: 4 additions & 2 deletions src/conversions/either.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,10 @@
//!
//! [either](https://docs.rs/either/ "A library for easy idiomatic error handling and reporting in Rust applications")’s

#[cfg(feature = "experimental-inspect")]
use crate::inspect::types::TypeInfo;
use crate::{
exceptions::PyTypeError, inspect::types::TypeInfo, FromPyObject, IntoPy, PyAny, PyObject,
PyResult, Python, ToPyObject,
exceptions::PyTypeError, FromPyObject, IntoPy, PyAny, PyObject, PyResult, Python, ToPyObject,
};
use either::Either;

Expand Down Expand Up @@ -97,6 +98,7 @@ where
}
}

#[cfg(feature = "experimental-inspect")]
fn type_input() -> TypeInfo {
TypeInfo::union_of(&[L::type_input(), R::type_input()])
}
Expand Down
Loading