style: clippy #66

somehowchris · 2022-05-05T10:40:08Z

Just some clippy suggestions + applying cargo clippy --allow-dirty --fix to remove things such as clones generated by codegen where a copy would be possible

simlay

I think it's worth mentioning that some of the stuff this PR modifies is generated in the codegen subcrate in a handlebars template.

somehowchris · 2022-05-05T20:45:59Z

Would you mind explaining what you exactly mean by “modifies”.

Yes, I have modified the files. i.e. it removes additions of @index if its 0 as this might not be optimized away by the compiler if it can’t detect what’s going on.

But in terms of modification of behavior, it should do the exact same as id + 3 + 0 is the same as id + 3.

But again, that’s a clippy suggestion.

ewilken

First: thanks a lot for your contribution! Great to have more hands on this!

I like the idea of running clippy on the auto-generated stuff, since there seem to be some actual small optimizations to gain with it.

However, please have a look at my comments, especially the testing after dependency update part as well as fixing the auto-generated tokio imports (and spacing) in the codegen templates rather than with clippy. (I feel like we could really use some integration tests on this at some point 😬)

ewilken · 2022-05-07T11:18:00Z

Cargo.toml

+aead = "0.4.3"
+async-trait = "0.1.35"
+byteorder = "1.4.3"
+bytes = "1.1.0"
+chacha20poly1305 = "0.9.0"
+ed25519-dalek = { version = "1.0.1", features = ["std", "serde"] }
+erased-serde = "0.3.20"
 eui48 = { version = "1.0", features = ["serde"] }
 futures = "0.3"
 get_if_addrs = "0.5"
 hkdf = "0.11"
-hyper = { version = "0.14", features = ["server", "http1"] }
+hyper = { version = "0.14.18", features = ["server", "http1"] }
 libmdns = "0.6"
-log = "0.4"
-num = "0.2"
+log = "0.4.17"
+num = "0.4.0"
 rand = "0.7"
-serde = { version = "1.0", features = ["rc", "derive"] }
-serde_json = "1.0"
+serde = { version = "1.0.137", features = ["rc", "derive"] }
+serde_json = "1.0.81"
 sha2 = "0.9"
 signature = "1.1"
 srp = "0.5"
 thiserror = "1.0"
-tokio = "1.8"
-url = "2.1"
-uuid = { version = "0.8", features = ["v4", "serde"] }
+tokio = "1.18.1"
+url = "2.2.2"
+uuid = { version = "1.0.0", features = ["v4", "serde"] }
 x25519-dalek = "0.6"

-[build-dependencies]
-handlebars = "2.0"
-serde = { version = "1.0", features = ["derive"] }
-serde_json = "1.0"
-uuid = { version = "0.8", features = ["v4", "serde"] }
-
 [dev-dependencies]
-env_logger = "0.8"
-serde = { version = "1.0", features = ["derive"] }
-tokio = { version = "1.8", features = ["rt-multi-thread", "time", "macros"] }
+env_logger = "0.9.0"
+serde = { version = "1.0.137", features = ["derive"] }
+tokio = { version = "1.18.1", features = ["rt-multi-thread", "time", "macros"] }


It's a matter of taste in the end, but I'd prefer keeping the patch semver blocks out of the Cargo.toml. Is there a reason you put them back in?

ewilken · 2022-05-07T11:20:48Z

Cargo.toml

+chacha20poly1305 = "0.9.0"
+ed25519-dalek = { version = "1.0.1", features = ["std", "serde"] }
+erased-serde = "0.3.20"
 eui48 = { version = "1.0", features = ["serde"] }
 futures = "0.3"
 get_if_addrs = "0.5"
 hkdf = "0.11"
-hyper = { version = "0.14", features = ["server", "http1"] }
+hyper = { version = "0.14.18", features = ["server", "http1"] }
 libmdns = "0.6"
-log = "0.4"
-num = "0.2"
+log = "0.4.17"
+num = "0.4.0"


IIRC there was a reason I didn't update some of the dependencies, e.g. chacha20poly1305 and num, yet, since there was something breaking on the pairing step handlers I didn't bother to look into yet. Did you test a whole pairing process after all these dependency updates? Is everything still working?

ewilken · 2022-05-07T11:21:29Z

codegen/Cargo.toml

+handlebars = "4.2.2"
+rayon = "1.5.2"
 serde = { version = "1.0", features = ["derive"] }
-serde_json = "1.0"
-uuid = { version = "0.8", features = ["v4", "serde"] }
+serde_json = "1.0.81"
+uuid = { version = "1.0.0", features = ["v4", "serde"] }


Same here, just two semver blocks?

ewilken · 2022-05-07T11:24:00Z

codegen/src/main.rs

+        let mut sorted_categories = m
+            .homekit
+            .categories
+            .par_iter()
+            .map(|(_, v)| v.clone())
+            .collect::<Vec<_>>();
+        sorted_categories.par_sort_by(|a, b| a.number.partial_cmp(&b.number).unwrap());
+
+        let mut sorted_characteristics = m
+            .hap
+            .characteristics
+            .par_iter()
+            .map(|(_, v)| v.clone())
+            .collect::<Vec<_>>();
+        sorted_characteristics.par_sort_by(|a, b| a.name.cmp(&b.name));
+
+        let mut sorted_services = m.hap.services.par_iter().map(|(_, v)| v.clone()).collect::<Vec<_>>();
+        sorted_services.par_sort_by(|a, b| a.name.cmp(&b.name));


Cool! Just out of curiosity, did you benchmark how much faster this got before vs. after rayon? 🤓 Never felt the need for optimizations like this, since you don't codegen that often anyway, but cool!

ewilken · 2022-05-07T11:26:09Z

examples/adding_accessories_dynamically.rs

@@ -1,5 +1,3 @@
-use tokio;


I think all of these obsolete tokio imports are something that should be fixed on the template inside the codegen crate rather than by clippy after the codegen step. This applies to all auto-generated files.

ewilken · 2022-05-07T11:26:49Z

examples/custom_characteristics_services_accessories.rs

@@ -2,7 +2,7 @@ use serde::{
    ser::{SerializeStruct, Serializer},
    Serialize,
 };
-use tokio;
+


Suggested change

ewilken · 2022-05-07T11:29:03Z

src/accessory/generated/mod.rs

+pub mod air_purifier;/// Air Quality Sensor accessory definition.
+pub mod air_quality_sensor;/// Carbon dioxide Sensor accessory definition.
+pub mod carbon_dioxide_sensor;/// Carbon monoxide Sensor accessory definition.
+pub mod carbon_monoxide_sensor;/// Contact Sensor accessory definition.
+pub mod contact_sensor;/// Door accessory definition.
+pub mod door;/// Fan accessory definition.
+pub mod fan;/// Fan v2 accessory definition.
+pub mod fan_v2;/// Garage Door Opener accessory definition.
+pub mod garage_door_opener;/// Humidifier-Dehumidifier accessory definition.
+pub mod humidifier_dehumidifier;/// Humidity Sensor accessory definition.
+pub mod humidity_sensor;/// Leak Sensor accessory definition.
+pub mod leak_sensor;/// Light Sensor accessory definition.
+pub mod light_sensor;/// Motion Sensor accessory definition.
+pub mod motion_sensor;/// Occupancy Sensor accessory definition.
+pub mod occupancy_sensor;/// Outlet accessory definition.
+pub mod outlet;/// Security System accessory definition.
+pub mod security_system;/// Smart Speaker accessory definition.
+pub mod smart_speaker;/// Smoke Sensor accessory definition.
+pub mod smoke_sensor;/// Stateful Programmable Switch accessory definition.
+pub mod stateful_programmable_switch;/// Stateless Programmable Switch accessory definition.
+pub mod stateless_programmable_switch;/// Switch accessory definition.
+pub mod switch;/// Temperature Sensor accessory definition.
+pub mod temperature_sensor;/// Thermostat accessory definition.
+pub mod thermostat;/// Wi-Fi Router accessory definition.
+pub mod wi_fi_router;/// Wi-Fi Satellite accessory definition.
+pub mod wi_fi_satellite;/// Window accessory definition.
+pub mod window;/// Window Covering accessory definition.


Isn't this breaking the docs?

ewilken · 2022-05-07T11:32:58Z

src/lib.rs

@@ -10,7 +10,6 @@ pub use crate::{
    pin::Pin,
    transport::bonjour::{BonjourFeatureFlag, BonjourStatusFlag},
 };
-


I'd keep this line.

ewilken · 2022-05-07T11:33:41Z

src/service/generated/access_code.rs

+			access_code_control_point: AccessCodeControlPointCharacteristic::new(id  + 1, accessory_id),
+			access_code_supported_configuration: AccessCodeSupportedConfigurationCharacteristic::new(id + 1  + 1, accessory_id),
+			configuration_state: ConfigurationStateCharacteristic::new(id + 2  + 1, accessory_id),


This looks like broken spacing to me.

Edit: this applies to all auto-generated services.

ewilken · 2022-05-07T11:34:14Z

src/service/generated/access_code.rs

@@ -33,7 +33,6 @@ pub struct AccessCodeService {
 	pub access_code_supported_configuration: AccessCodeSupportedConfigurationCharacteristic,
 	/// Configuration State characteristic (required).
 	pub configuration_state: ConfigurationStateCharacteristic,
-


I'd keep this line.

Edit: this applies to all auto-generated services.

style: clippy

7cd7b99

simlay reviewed May 5, 2022

View reviewed changes

ewilken requested changes May 7, 2022

View reviewed changes

somehowchris closed this May 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: clippy #66

style: clippy #66

somehowchris commented May 5, 2022

simlay left a comment

somehowchris commented May 5, 2022 •

edited

Loading

ewilken left a comment

ewilken May 7, 2022

ewilken May 7, 2022

ewilken May 7, 2022

ewilken May 7, 2022

ewilken May 7, 2022

ewilken May 7, 2022

ewilken May 7, 2022

ewilken May 7, 2022

ewilken May 7, 2022

ewilken May 7, 2022

ewilken May 7, 2022

ewilken May 7, 2022

style: clippy #66

style: clippy #66

Conversation

somehowchris commented May 5, 2022

simlay left a comment

Choose a reason for hiding this comment

somehowchris commented May 5, 2022 • edited Loading

ewilken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somehowchris commented May 5, 2022 •

edited

Loading