Style: fix the findings of the codebase review at a320c3f

Open
#381 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
3/5
Estimated time
1-2 days
Newbie friendliness
35/100
Issue type
Refactor
Clarity
Mostly clear
Activity status
Active
Tech stack
rust

Research direction

This is an umbrella checklist, so pick one rule and one section, for example the blank line before the closing Ok(...) in tellus/src/backoff.rs:36, after reading the conventions in AGENTS.md. Fix only the listed places, and open a pull request that says Part of #381. The issue asks that this wait until the 0.6 breaking changes land, so check that first. Done means every listed location for that rule is fixed with no behavior change, and the checklist box is ticked.

Written by the indexing model from the issue text.

Description

Style findings from a review of the codebase at a320c3f, against the conventions in AGENTS.md. None of them changes behavior or a public name; the renames of public items are in #346. Best done one rule at a time, each PR Part of this issue, after the breaking changes of 0.6 have landed, since they touch the same files.

Across the workspace

  • Blank line between a block and the closing Ok(...): 35 places, against 168 which follow the rule.
    • tellus/src: backoff.rs:36, host.rs:225, host.rs:371, persistence/in_memory_store.rs:68, :113, cluster/bootstrap.rs:287, cluster/endpoint.rs:1863, cluster/failure.rs:89, cluster/peer.rs:1478, :1697, cluster/reply.rs:671, cluster/sink.rs:171, cluster/transport/quic.rs:642.
    • Other crates: tellus-bootstrap-k8s/src/lib.rs:72, :221, tellus-bootstrap-k8s/tests/k8s_seeds.rs:193, tellus-cluster-inspect/src/lib.rs:146, tellus-comparison/benches/frameworks.rs:679, :750, tellus-comparison/src/bin/report.rs:60, :261, :433.
    • Benches: tellus/benches/messaging.rs:411, :541, persistence.rs:189, stash.rs:213, :293.
    • Tests: tellus/tests/host.rs:492, host_persistence.rs:123, persistence.rs:1531, supervision.rs:546, :589, :728, termination.rs:727, test_context.rs:459.
  • super:: imports: tellus/src/cluster/reachability.rs:371 is the only one in a tellus/src test module; 41 files there have test modules and the rest import through crate::. use super::* in tellus-cluster-inspect/src/lib.rs:232, events.rs:327, recorder.rs:118 and tellus-cluster-demo-tui/src/model.rs:670, sse.rs:68, ui.rs:465.
  • std::sync::mpsc::... written in full at tellus/src/cluster/endpoint.rs:3487, tellus/tests/ask.rs:66, :134, :287, :416, :719, supervision.rs:233, :565, :608, termination.rs:85, :387. None of these files has a second mpsc in scope which would force the long path.
  • slots_by_key in tellus/src/timers.rs:238 is a HashMap<&'static str, usize>. Open: does a map keyed by key count as indexed by key under the _by_x rule?

Core (paths under tellus/src unless given in full)

  • Item order. stash.rs:487-488: private apply_requests sits between pub(crate) functions. mailbox.rs:115-123: impl TerminatedHandler for CountedSender<Incoming<M>> lives in mailbox.rs, while CountedSender is defined in quota.rs.
  • Visibility wider than needed. quota.rs: Quota::reserve (:94), reserve_forced (:107), Reservation and commit (:124-130). stash.rs: STASHED_FAILED_TO_DROP (:100), Stash::new, settle, note_timer_item, drop_signals_of, hand_back_all, take_all. timers.rs: TimerTable::take_all (:340).
    Confidence: Confirmed by reading (grep over tellus/src).
  • Item docs naming dependents: watch.rs:8-10 (MailboxHandle, ActorContext), quota.rs:205-206, :252-254 ("lets a host drain what it buffered"), :297-298, stash.rs:613-614 (recovered), and the // comment at timers.rs:514 (TestContext).
  • Multi-line docs on non-pub items which explain rather than constrain: actor_system.rs:127-129, actor_context.rs:924-927, :428-429, :594-595, actor_ref.rs:71-77. Mixed /// and // for the same kind of note within one file: actor_context.rs:412, :872 against :814, :855.
  • tellus/src/quota.rs:299. An empty line separates the doc comment at :297-298 from #[test] at :300.
    Confidence: Plausible (clippy not run).
  • # Errors wording puts a consequence under the heading: actor_context.rs:283 (", returning the incoming, so the actor decides what to do with it"), actor_system.rs:77 (", which no ordinary termination causes").
  • mailbox.rs:71 turns the receive error into an Option with .ok()?, while try_recv at :88 matches the variants. host.rs:366 writes tokio::spawn where the crate otherwise writes task::spawn.

Persistence (paths under tellus/src/persistence unless given in full)

  • Item order. spawn.rs: private Recovered (154), Settled (160), ReplayError (167), SnapshotError (176) and spawn_event_sourced (323) precede the pub(crate) functions at 348-500; pub(crate) fn apply_events (796) follows the private functions. store.rs: EncodedEvent (113) precedes StoredEvent (127), which contains it; likewise EncodedSnapshot (137) and StoredSnapshot (152). persistence_id.rs: PersistenceIdSegment (84) precedes InvalidPersistenceId (96). tellus-persistence-postgres/src/lib.rs: is_unique_violation (301) is called by append but sits last.
  • Comments which explain: spawn.rs:769-770, in_memory_store.rs:81-82, tellus-persistence-postgres/src/lib.rs:97. spawn.rs:187-188 has a /// on the private macro run_event_sourced, the only documented non-public item in that file.
  • tellus/src/persistence.rs:22-24. pub(crate) on event_store, snapshot_store and codec is not needed: the only readers are in persistence/spawn.rs, a descendant module.
  • Longer paths than needed: tellus-persistence-postgres/src/lib.rs:248 (sqlx::migrate::MigrateError), tellus-persistence-postgres/tests/store.rs:53, :59, :65, :357, :358, :435-437 (tellus::persistence_tests::...), tellus-persistence-postgres/examples/event_sourced_supervision.rs:60-63.

Cluster: peer and frames (paths under tellus/src/cluster)

  • Item order. frame.rs:111: mod payload_bytes between enum Frame (30) and impl Frame (164). node.rs:9: pub(crate) struct NodeId before pub struct Incarnation (46) and pub struct ClusterId (60). peer.rs:777: backoff_or_give_up before connect (798) and write_streams (887); admit_inbound (1295) behind read_frames (1142). peer.rs:2808-2809: no blank line between a test and fn addr.
  • Paths: frame.rs:137 (std::fmt::Result), peer.rs:1809 and tests :2132, :2633, :2667 (crate::sync::lock(&super::DEAD_LETTERS)), peer.rs:2141 (std::future::pending()), :4856-4857 (std::time::Instant::now()).
  • Naming and order. One Generation has three names: id (lanes.rs:244), lane_id (lanes.rs:198, peer.rs:64), generation (lanes.rs:135, :152, :158, :165). matching_lane (lanes.rs:277) returns a CountedSender, not a lane. peer.rs:836-848 and :1727-1733: generics <S, R>, where clause lists R first. peer.rs:507-509 puts the receiver before the sender, the tuple at :460-462 the other way.
  • wire.rs:173-174, :182-183: comments naming "the trait object codec this one replaced", which no longer exists.

Cluster: endpoint and membership (paths under tellus/src/cluster)

  • Item order. membership.rs: private MemberRepr (89-110) among pub items; pub fn members (302) and pub fn down (315) follow pub(crate) enum JoinRound (208); pub(crate) const MIN_FRAME_SIZE (540) is not at the top; Merge, DownOutcome, JoinAdmission (567-596) follow pub(crate) functions; impl MemberEntry (955) is separated from its struct (942) by Scan (948). endpoint.rs: pub fn start_endpoint (565) and pub fn form (609) precede pub enum Lifecycle (621); private UncheckedEndpointConfig (402) among pub types; private fn validate (1808) precedes pub(crate) fn get (1866); private inbound_closed (1395) among pub(crate) functions.
  • impl Trait arguments: endpoint.rs:784-787, membership.rs:384, where the rest of both files uses a type parameter with a where clause.
  • Visibility wider than needed: Member::MAX_ENCODED_LEN (membership.rs:83), EndpointInner::offer_snapshot (893), push_reachability (1203), untrack_idle_peers (1451).
  • state.rs:32-40 orders the fields version, this, cluster, ...; ClusterState::new (:154-162) takes this, cluster, version, ....
  • Paths in endpoint.rs tests: :2681, :3474 (tokio::time::timeout), :3487, :3489, :3497, :3504, :3525 (std::thread::spawn).
  • Comments which explain: membership.rs:646, endpoint.rs:933-934, :1013-1014, :1616-1617.
  • Collection names in the singular: Departures::inbound and deferred (endpoint.rs:1775-1776), UnreachableWarnings::warned_at (2242). MemberEntry::up() (membership.rs:955-961) reads Instant::now() although its callers are handed now. Missing blank line before the doc comment at endpoint.rs:4088.

Cluster: directory and replies (paths under tellus/src/cluster)

  • Item order. receptionist.rs:51-114: pub free functions precede the pub types Settlement (119), Subscription (143), ReceptionistError (185). :788, :795: PublishedChange and SnapshotSource after the free functions; :799 impl SnapshotSource for Receptionist is 370 lines from Receptionist (198). :558: private Partial inside the pub(crate) tier. reply.rs:577, :584: Cancellation and ProxyOrigin follow the pub(crate) free functions. schema.rs:132: pub(crate) fn new precedes the pub methods of Peer.
  • tellus/src/cluster/discovery.rs:211-228. Nonce lives in discovery.rs, which does not use it; only reply.rs, frame.rs and lanes.rs do.
  • Visibility wider than needed: PendingReplies::discard_ask, cancel_proxy, fail_recipient, AskReplyTags, ReplyToError in reply.rs; Receptionist::listing, applied_version and Entry in receptionist.rs.
  • Mechanical nits. (this issue owns part b: the remaining nits, Key::topic and Key::of_kind included (not pub))
    • impl Trait arguments: discovery.rs:38, :42, :50, :173, pubsub.rs:42, receptionist.rs:402.
    • super:: path: schema.rs:256.
    • Bare type_name in schema.rs:15, reply.rs:19 against any::type_name in manifest.rs:55, discovery.rs:65.
    • watch.rs:314 turns an error into an Option; watch.rs:330 discards a result.
    • Comments describing the removed codec: codec.rs:102, :195, wire.rs:173, :182, reply.rs:880, :1605, and the test names *_matches_the_trait_object_codec.
    • Bare-noun constructors: Key::topic, Key::of_kind (discovery.rs:42, :50).
    • Blank line before the closing Ok(...): registry.rs:146, reply.rs:849, beside the swept reply.rs:671.

Cluster: failure detection, formation, bootstrap and transport (paths under tellus/src/cluster unless given in full)

  • Style. (this issue owns part b: the remaining style)
    • Impl away from its type: failure.rs:393 (type at 134), reachability.rs:98 (type at 43).
    • const not at the top: reachability.rs:36, :41. pub(crate) fn snapshot_chunks (:352) after private items.
    • Private Unchecked* types and their TryFrom impls inside the pub tier: failure.rs:106-117, :348-391, bootstrap.rs:156-198, k8s lib.rs:178-204.
    • impl Into<String> arguments: quic.rs:84, :179, dns lib.rs:60, :70.
    • Paths: quic.rs:949, :1018, :1021, :1030, :1308, bootstrap.rs:1083.
    • Comments which explain: bootstrap.rs:306, :472.

Inspect, demo and TUI (paths under tellus-cluster-inspect, tellus-cluster-demo or tellus-cluster-demo-tui, named in the item)

  • tellus-cluster-inspect/Cargo.toml:19, :22. serde_json and the tokio features macros and time are used only by tests but are normal dependencies. tellus-cluster-demo/Cargo.toml:18 has the same serde_json case.
    Confidence: Confirmed by reading (grep).
  • Paths: events.rs:279 (std::iter::once), sse.rs:31 (std::mem::take), recorder.rs:255, events.rs:502, lib.rs:356, verifier.rs:902, ui.rs:161, :166, :167, :171.
  • tellus-cluster-inspect/src/lib.rs:155: trait Sources: Send + Sync has its supertrait bound inline. The demo's Probing (node.rs:208) uses a where clause.
  • Item order. model.rs:531: private NodeSource::new before pub fn label (553). ui.rs: body (43) before verifier_strip (74), against the call order in draw. sources.rs: client() is called first and sits last (182). node.rs: log_changes (867) is called from main before run (531).
  • events.rs:175 Replay { resumed, anchor, entries } against recorder.rs:104 Capture { resumed, items, anchor } and events.rs:263 sse(log, items, anchor, ..).
  • Swallowed errors: events.rs:254-255 (.ok() twice, documented behavior), sources.rs:172-177 (poll drops request and decode errors with no log).
  • Doc comment on a single non-public item: lib.rs:165-166 (Endpoint::snapshot), events.rs:153 (read_after).

Examples, benches and comparison (paths under tellus/examples unless given in full)

  • tellus/Cargo.toml:155-156, :213, :218 list "persistence" beside "persistence-in-memory", which implies it; the event-sourced tutorial entries list only the latter. work_pulling is the one example without an [[example]] entry.
  • Paths: tracing_subscriber::fmt::layer() in all 14 init_tracing copies, shut_down_gracefully.rs:76, secure_with_mutual_tls.rs:94-95, 04_remote_watch.rs:217.
  • Swallowed errors: shut_down_gracefully.rs:143 (.map_err(|_| StatusCode::SERVICE_UNAVAILABLE)), report.rs:374-376.
  • Item order: profile.rs:79, benches/messaging.rs:252-258, shut_down_gracefully.rs:157, :169, frameworks.rs:28, :35.
  • Doc comments on private items, mixed within a file: cluster.rs:123, :176, :252, :270, :307, :327, :352, :386, :414, remote_scatter_gather.rs:275, :294, benches/messaging.rs:287, :294, :328, :337. /// carrying a constraint or a maintenance note: frameworks.rs:498, :711, report.rs:436.
  • Uniformity across examples.
    • NonZero..::new(n).unwrap() at retry_writes.rs:27, defer_messages.rs:14, work_pulling.rs:32, against .expect("3 is not zero") in the tutorials.
    • The context type is spelled three ways: 08_stale_readings.rs:379, retry_writes.rs:78, defer_messages.rs:66.
    • enum Command for a plain actor at 07_device_manager.rs:144, beside GroupMessage, DeviceMessage, QueryMessage.
    • reserved_addrs(2) literal in clustered 02 and 03, against NODES in 01, 04, 05.
    • "## " output prefix only in cluster.rs, remote_scatter_gather.rs, work_pulling.rs.

Tests (paths under tellus/tests unless given in full)

  • Helpers reimplemented per file: recv and assert_terminates (ask.rs:335, watch.rs:241, termination.rs:317, persistence.rs:1096), next_event and stop (pipe_to_self.rs:235, timers.rs:1296, stash.rs:753), restart_config (ask.rs:352, stash.rs:646, persistence.rs:1129).
  • Two reporting idioms: mpsc::channel(1) with let _ = self.0.try_send(..), which drops a second report silently, against unbounded_channel with .expect("test dropped the receiver").
  • Mechanical nits.
    • super:: paths: stash.rs:796, :798, :809, :823, :825.
    • Three spellings of the std channel: std::sync::mpsc::..., mpsc as std_mpsc (termination.rs:8), sync::mpsc (timers.rs:227).
    • tokio::spawn at fairness.rs:24, :59, termination.rs:165, timers.rs:323, test_context.rs:278, host.rs:477.
    • impl Trait argument: fairness.rs:83.
    • Assertion helpers without the assert_ prefix: supervision.rs:296, :308.
    • termination.rs:153 tests the cooperative budget, the subject of fairness.rs.
    • supervision.rs:172 says min_backoff and max_backoff; the fields are backoff.min() and backoff.max().
    • Blank line before the closing Ok(...): watch.rs:396, beside the swept ones.

Cluster test, justfile and CI

  • Style of tellus/tests/cluster.rs. std::io::stdout() and std::io::stdin() at 15 places although io is imported; std::future::pending at 361, 645, 693. impl Manifest for Note (1400) away from struct Note (1089). The doc at 2168-2171 describes the echo scenario but sits on schema_evolution_scenario. Swallowed errors at 349, 5844, 1337, 1386. Blank line before the closing Ok(()) at 1706. own() and own_addr(runtime, echo) return different nodes' addresses.

Found by reading the code at a320c3f; nothing was run. Line numbers refer to that commit.

Dominant language
Rust
Stars
37
Forks
1
Avg merge
6h 20m
Merged PRs (30d)
135

Getting set up

  • Ships a Dockerfile or Docker Compose file
  • No pull request template
  • No contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from tellus-rs/tellus

All issues in tellus-rs/tellus

Similar issues

More Rust issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.