docs/tests: fix rustdoc links and appease clippy strict lints #1576
Conversation
| { | ||
| /// Checks if the permutation is correct | ||
| pub fn from_indices(v: Vec<usize>) -> Result<Self, ()> | ||
| pub fn from_indices(v: Vec<usize>) -> Option<Self> |
There was a problem hiding this comment.
I understand that you didn't "change" anything, this is mostly reformatting, but this one here is different. This is a public function, so there might be several callers out there who will be surprised by this API change.
I'm not against changing it. I guess a Result<X, ()> is functionally equivalent to an Option<X>. I just want to highlight this change.
There was a problem hiding this comment.
Personally I'm ok with this, but I'm glad you brought it up
There was a problem hiding this comment.
Let me know if you would like me to revert it.
https://effective-rust.com/transform.html
I think its better to return the Option and makes the signature clearer, and should require minimal refactoring downstream. While Result<T, ()> is basically the same functionally as Option<T> it doesn't covey the intent clearly. Usually Result is reserved for situations like where there is an expected value or an error variant and allows for propagating errors with ?.
There was a problem hiding this comment.
Agreed that it's the better practice, just good for us to keep in mind it will have to be a breaking change release.
|
@RPG-Alex same question as before, is this just another round of |
There was a problem hiding this comment.
Why did a large block of code move in this file?
There was a problem hiding this comment.
Modules shouldn't be implemented after tests: https://rust-lang.github.io/rust-clippy/master/index.html#items_after_test_module
error: items after a test module
--> src/linalg/impl_linalg.rs:973:1
|
973 | mod blas_tests
| ^^^^^^^^^^^^^^
...
1104 | / impl Dot<ArrayRef<A, IxDyn>> for ArrayRef<A, IxDyn>
1105 | | where A: LinalgScalar
| |_____________________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#items_after_test_module
= note: -D clippy::items-after-test-module implied by -D warnings
= help: to override -D warnings add #[allow(clippy::items_after_test_module)]
= help: move the items to before the test module was defined
error: could not compile ndarray (lib test) due to 1 previous error
There was a problem hiding this comment.
To my knowledge, there is no Rust-mandated or even Rust-suggested TOML formatter, is there? So these changes could be undone as soon as I hit save on my Cargo.toml? If we want to enforce formatting for non-Rust files, we'll need to do it with something more standardized.
There was a problem hiding this comment.
I use taplo: https://github.com/tamasfe/taplo
and just run taplo fmt in project and then integrate into CI.
To my knowledge, there is no Rust-mandated or even Rust-suggested TOML formatter, is there? So these changes could be undone as soon as I hit save on my
Cargo.toml? If we want to enforce formatting for non-Rust files, we'll need to do it with something more standardized.
If you put it into the CI it would be standardized for all future PRs. I'm not sure its necessary for this project but I run it locally out of habit and see that it removed mostly extra whitespace. I don't know that changes would be undone if you changed the Cargo.toml though.
| #[allow(dead_code)] | ||
| /// Merge if they are in order (left to right) and contiguous. | ||
| /// Skips merge if T does not need drop. | ||
| pub(crate) fn try_merge(mut left: Self, right: Self) -> Self |
There was a problem hiding this comment.
Under what check is this dead code? I can clearly see that it is called inside the zip_impl macro
There was a problem hiding this comment.
Same question for several other parallel-related dead code annotations
There was a problem hiding this comment.
Under what check is this dead code? I can clearly see that it is called inside the
zip_implmacro
It is feature gated:
#[cfg(feature = "rayon")]
#[allow(dead_code)]https://doc.rust-lang.org/std/macro.cfg.html
In Cargo.toml:
rayon = { version = "1.10.0", optional = true }Rayon is an optional feature and therefore used when added at compile time, I was following the convention here, though it may not be necessary from your feedbacK?
Line 12 in 49cbd5d
Happy to remove them. Please let me know.
If you want to add it to the CI then be aware that the strictest setting is still sending errors: error: this range is empty so it will yield no values
--> benches/bench1.rs:746:35
|
746 | let a = a.slice_mut(s![1..-1, 1..-1]);
| ^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#reversed_empty_ranges
help: consider using the following if you are attempting to iterate over this range in reverse
|
746 - let a = a.slice_mut(s![1..-1, 1..-1]);
746 + let a = a.slice_mut(s![1..-1, (-1..1).rev()]);This one is odd and I was considering opening an isuse about these tests as |
3 participants
Tighten linting; fix rustdoc warnings across feature gates
I continued using stricter linting and found several odd cases in tests and
examples, along with rustdoc warnings triggered by feature-gated items.
This commit cleans up those warnings by:
linking to non-exported items
All tests are still passing.
Clippy still warns about the use of
1..-1as an empty range; this appearsintentional in ndarray slicing semantics, so I’ve left it unchanged for now.
Please review the rustdoc changes that rely on feature-gated documentation.