Fix clippy warnings by i1i1 · Pull Request #500 · paritytech/substrate-telemetry

i1i1 · 2022-09-23T12:53:07Z

No description provided.

backend/common/src/ws_client/receiver.rs

backend/telemetry_core/src/find_location.rs

niklasad1 · 2022-09-26T08:44:01Z

backend/telemetry_core/src/state/chain.rs

-                if let Some(timestamp) = self.timestamp {
-                    self.block_times.push(now.saturating_sub(timestamp));
-                    self.average_block_time = Some(self.block_times.average());
+            match block.height.cmp(&self.best.height) {


I'm not a fan of this change if think the old code was easier to read.

which clippy lint is this?

This one:
https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain

backend/telemetry_shard/src/main.rs

backend/test_utils/src/server/channels.rs

niklasad1 · 2022-10-10T12:18:44Z

backend/common/src/byte_size.rs

-            ("1Mi", 1 * 1024 * 1024),
-            ("1GiB", 1 * 1024 * 1024 * 1024),
-            ("1Gi", 1 * 1024 * 1024 * 1024),
-            (" 1 Gi ", 1 * 1024 * 1024 * 1024),


these are just silly, so much more readable before :)

perhaps we could should add a clippy config for style lints like this one.

I'd be up for a clippy config being added to telemetry so that we can keep more of what we like; I'm generally happy with these changes but there isn't much value in adding Default impls and a couple of other nits I have (but I htink nits are allowed since that's what clippy is all about!)

jsdw · 2022-10-10T12:21:04Z

backend/common/src/assign_id.rs

+impl<Id, Details> Default for AssignId<Id, Details>
+where
+    Details: Eq + Hash,
+    Id: From<usize> + Copy,
+    usize: From<Id>,
+{
+    fn default() -> Self {
+        Self::new()
+    }
+}


I guess #[derive(Default)] didn't work? I'd prefer that if possible :)

BiMap doesn't impl Default

jsdw · 2022-10-10T12:24:40Z

I'm ok with most of these changes, but perhaps we should sit down, make an issue and write a proper clippy config as @niklasad1 suggests so that we can decide what we want to ignore/change in telemetry and then run over and this the warnings there.

@niklasad1 are you up for doing that when you have some time?

niklasad1 · 2022-10-10T12:32:54Z

backend/telemetry_core/src/aggregator/inner_loop.rs

        local_id: ShardNodeId,
        ip: std::net::IpAddr,
-        node: common::node_types::NodeDetails,
+        node: Box<common::node_types::NodeDetails>,


std::mem::size_of::<NodeDetails>() == 111

I think that is small enough to avoid to Box this one

Yeah, but enum variant is around 369 bytes:

warning: large size difference between variants --> telemetry_core/src/aggregator/inner_loop.rs:48:1 | 48 | / pub enum FromShardWebsocket { 49 | | /// When the socket is opened, it'll send this first 50 | | /// so that we have a way to communicate back to it. 51 | | Initialize { ... | 55 | / | Add { 56 | | | local_id: ShardNodeId, 57 | | | ip: std::net::IpAddr, 58 | | | node: common::node_types::NodeDetails, 59 | | | genesis_hash: common::node_types::BlockHash, 60 | | | }, | |_|_____- the largest variant contains at least 369 bytes 61 | | /// Update/pass through details about a node. 62 | / | Update { 63 | | | local_id: ShardNodeId, 64 | | | payload: Box<node_message::Payload>, 65 | | | }, | |_|_____- the second-largest variant contains at least 16 bytes ... | 69 | | Disconnected, 70 | | } | |_^ the entire enum is at least 376 bytes | = note: `#[warn(clippy::large_enum_variant)]` on by default = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant help: consider boxing the large fields to reduce the total size of the enum | 58 | node: Box<common::node_types::NodeDetails>, | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

niklasad1 · 2022-10-10T12:36:24Z

backend/telemetry_core/src/aggregator/inner_loop.rs

    Update {
        local_id: ShardNodeId,
-        payload: node_message::Payload,
+        payload: Box<node_message::Payload>,


std::mem::size_of::<Payload>() == 344, I'm still in favor to avoid to box this but not sure where to cut the threshold without benching it ^^.

Clippy seems to use 200 as default, https://rust-lang.github.io/rust-clippy/master/#large_enum_variant

niklasad1

nice work, my comments are towards the default clippy settings which we could take care of it in another PR as @jsdw said.

niklasad1 · 2022-10-10T12:48:47Z

@niklasad1 are you up for doing that when you have some time?

Sure, it seems like I'm the grumpy one against the default clippy warnings so sure :)

jsdw · 2022-10-10T13:39:40Z

I'd suggest just starting with this PR if it's useful, adding some clippy rules, adding the CI check too (since this PR doesn't have it) and making a new PR. Then we can close this one since it's conflicting at the mo anyway.

But whatever is easiest!

i1i1 added 2 commits September 23, 2022 15:37

cargo clippy --fix --all --all-targets

80fffc9

Fix rest of clippy warnings

4660ab0

i1i1 requested a review from a team as a code owner September 23, 2022 12:53

i1i1 mentioned this pull request Sep 23, 2022

Omit more data related to node autonomys/substation#34

Merged

niklasad1 reviewed Sep 26, 2022

View reviewed changes

backend/common/src/ws_client/receiver.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Sep 26, 2022

View reviewed changes

backend/telemetry_core/src/find_location.rs Show resolved Hide resolved

niklasad1 reviewed Sep 26, 2022

View reviewed changes

backend/telemetry_shard/src/main.rs Show resolved Hide resolved

niklasad1 reviewed Sep 26, 2022

View reviewed changes

backend/test_utils/src/server/channels.rs Outdated Show resolved Hide resolved

Remove useless .map_err calls

3a204ed

i1i1 requested a review from niklasad1 September 26, 2022 09:59

niklasad1 reviewed Oct 10, 2022

View reviewed changes

jsdw reviewed Oct 10, 2022

View reviewed changes

niklasad1 reviewed Oct 10, 2022

View reviewed changes

niklasad1 approved these changes Oct 10, 2022

View reviewed changes

Conversation

i1i1 commented Sep 23, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsdw commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 left a comment

Choose a reason for hiding this comment

Uh oh!

niklasad1 commented Oct 10, 2022

Uh oh!

jsdw commented Oct 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jsdw commented Oct 10, 2022 •

edited

Loading