Ensure CI covers examples and unit tests, fix clippy findings #191
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
djc
approved these changes
Nov 14, 2023
djc
reviewed
Nov 14, 2023
Per the docs[0], `--all` is a deprecated alias for `--workspace`, and `--workspace` is the default for directories that contain a workspace. This commit removes all `--all` instances since the default will do what we want and lets us avoid using deprecated command flags. [0]: https://doc.rust-lang.org/cargo/commands/cargo-test.html
The previous invocations were **not** checking example code for build errors, and if any unit tests were specified in examples they would be skipped.
Fixes:
```
warning: digits of hex, binary or octal literal not in groups of equal size
--> rcgen/tests/openssl.rs:244:35
|
244 | if openssl::version::number() >= 0x1_01_01_00_f {
| ^^^^^^^^^^^^^^ help: consider: `0x1010_100f`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unusual_byte_groupings
= note: `#[warn(clippy::unusual_byte_groupings)]` on by default
```
Resolves clippy findings related to references being created and immediately dereferenced by the compiler, or usage of `as_ref()` that had no effect.
Fixes:
```
warning: single-character string constant used as pattern
--> rcgen/src/lib.rs:1872:26
|
1872 | assert!(!pem.contains("\r"));
| ^^^^ help: try using a `char` instead: `'\r'`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern
= note: `#[warn(clippy::single_char_pattern)]` on by default
```
Fixes:
```
warning: length comparison to zero
--> rcgen/tests/openssl.rs:82:6
|
82 | if r_sl.len() == 0 {
| ^^^^^^^^^^^^^^^ help: using `is_empty` is clearer and more explicit: `r_sl.is_empty()`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
= note: `#[warn(clippy::len_zero)]` on by default
```
Fixes:
```
warning: this is a decimal constant
--> rcgen/tests/botan.rs:235:48
|
235 | params.not_after = rcgen::date_time_ymd(3016, 01, 01);
| ^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#zero_prefixed_literal
help: if you mean to use a decimal constant, remove the `0` to avoid confusion
|
235 | params.not_after = rcgen::date_time_ymd(3016, 1, 01);
| ~
help: if you mean to use an octal constant, use `0o`
|
235 | params.not_after = rcgen::date_time_ymd(3016, 0o1, 01);
| ~~~
```
In two places there were lifetimes that were either: a) not used b) could be ellided
Fixes:
```
error: use of `format!` to build up a string from an iterator
--> rcgen/examples/rsa-irc.rs:30:25
|
30 | let hash_hex: String = hash.as_ref().iter().map(|b| format!("{:02x}", b)).collect();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
help: call `fold` instead
--> rcgen/examples/rsa-irc.rs:30:46
|
30 | let hash_hex: String = hash.as_ref().iter().map(|b| format!("{:02x}", b)).collect();
| ^^^
help: ... and use the `write!` macro here
--> rcgen/examples/rsa-irc.rs:30:54
|
30 | let hash_hex: String = hash.as_ref().iter().map(|b| format!("{:02x}", b)).collect();
| ^^^^^^^^^^^^^^^^^^^^
= note: this can be written more efficiently by appending to a `String` directly
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#format_collect
= note: `-D clippy::format-collect` implied by `-D warnings`
```
The existing clippy findings for the examples have been addressed. This commit removes the `allow` configuration for `rsa-irc-openssl.rs` and `rsa-irc.rs` so that we'll catch new warnings as clippy improves. This will help keep the examples idiomatic and free of errors.
This ensures clippy covers examples, and unit test code.
cpu
force-pushed
the
cpu-test-all-targets
branch
from
7521f4d to
ed27b3e
Compare
est31
reviewed
Nov 19, 2023
| let inner = self.inner.borrow_mut(); | ||
| let r_sl = &inner.0[1 - self.end_idx][self.read_pos..]; | ||
| if r_sl.len() == 0 { | ||
| if r_sl.is_empty() { |
Member
There was a problem hiding this comment.
this is one of the clippy lints that I don't like :(.
est31
approved these changes
Nov 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While reviewing #188 I wanted to confirm that example code was being built in CI. It turns out that it wasn't. Similarly we haven't been running
clippyagainst test code, and so there was a number of findings to address.This branch updates CI to:
--all. This is a deprecated alias for--workspace, and--workspaceis the default for a directory containing a workspace so it can be omitted.--all-targetswhenever we runcargo check,cargo testorcargo clippy. This ensures coverage for both examples and unit tests.In order for the
cargo clippy ... --all-targetsto succeed this branch addresses each of the findings that were present. I've done this with a separate commit per class of finding to make it easier to review. In one case (7bfe0ef) I allowed the finding instead of fixing it since it seemed like the choice of digit groupings was done intentionally.