Skip to content

Commit 6ff580b

Browse files
committed
style(rust): drop the comments that narrated this code's history
A comment describes the code as it is. The history belongs in the commit message, where it is attached to the diff that makes it true; in the source it is stale for everyone who never saw the before. The comment above the `timeout` parse goes entirely: the field's doc comment already states the default. The zero rate-limit check states its constraint instead, and the three test comments say what they pin. Reported on the pull request as "No need to state the history of this code."
1 parent 2910b18 commit 6ff580b

2 files changed

Lines changed: 7 additions & 13 deletions

File tree

packages/rust/armonik-transport/src/config.rs

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -287,9 +287,6 @@ impl ClientConfig {
287287
)
288288
};
289289

290-
// `None`, not `Some(60s)`: nothing applied this until now, so the 60s was never observable, and
291-
// keeping it while starting to apply it would deadline every existing caller's requests at one
292-
// minute. `connect_timeout` above is the opposite case, its 60s having always been applied.
293290
let timeout = if timeout.is_empty() {
294291
None
295292
} else {
@@ -677,8 +674,8 @@ mod tests {
677674

678675
#[test]
679676
fn a_zero_rate_limit_is_rejected_rather_than_left_to_panic() {
680-
// `tower`'s `Rate::new` asserts both halves are above zero, and this is the change that starts
681-
// handing them to it: without this, `0/1s` panics inside `connect` instead of being reported.
677+
// `tower`'s `Rate::new` asserts both halves are above zero, so a zero has to be refused here
678+
// rather than reaching it: a panic inside `connect` tells the caller nothing.
682679
for value in ["0/1s", "1/0s", "0/0s"] {
683680
let error = ClientConfig::from_config_args(ClientConfigArgs {
684681
rate_limit: String::from(value),

packages/rust/armonik-transport/tests/timeout.rs

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
1-
//! `GrpcClient__Timeout` and `GrpcClient__RateLimit`, now that they reach the channel.
1+
//! `GrpcClient__Timeout` and `GrpcClient__RateLimit` reaching the channel.
22
//!
3-
//! A test that only asserted the parsing would have passed before this change too, since both options
4-
//! were already parsed and then dropped. So these go through a real connection to a real server.
3+
//! Through a real connection to a real server, measuring what the caller gets: asserting on the parsing
4+
//! alone would say nothing about whether either option reaches the channel.
55
66
mod common;
77

@@ -11,8 +11,7 @@ use common::{call, config, serve, SlowService};
1111

1212
#[tokio::test]
1313
async fn a_request_timeout_ends_a_call_the_server_is_too_slow_to_answer() {
14-
// The server takes ten seconds; the caller allows 300ms. Before this change the option was parsed
15-
// and ignored, so the call waited for the full ten.
14+
// A server that takes ten seconds against a caller that allows 300ms.
1615
let endpoint = serve(SlowService::new(Duration::from_secs(10))).await;
1716

1817
let channel = armonik_transport::connect(config(&endpoint, |args| {
@@ -76,9 +75,7 @@ async fn a_rate_limit_is_accepted_and_still_lets_calls_through() {
7675

7776
#[test]
7877
fn an_empty_timeout_means_no_timeout_rather_than_a_minute() {
79-
// The field is documented as defaulting to no timeout, and nothing applied it until now, so the
80-
// `Some(60s)` it used to parse to was never observable. This is the assertion that keeps a
81-
// one-minute deadline from appearing on every request of every caller.
78+
// What keeps a one-minute deadline off every request of every caller who set nothing.
8279
let config = config("http://localhost:5001", |_| {});
8380

8481
assert_eq!(config.timeout, None);

0 commit comments

Comments
 (0)