antoyo / relm

Idiomatic, GTK+-based, GUI library, inspired by Elm, written in Rust
MIT License
2.41k stars 79 forks source link

Fixes clippy warnings #275

Closed sanpii closed 3 years ago

sanpii commented 3 years ago

Fixes #239 & #254

This PR is incomplete, there is still 4 warnings:

``` Checking relm-derive v0.21.0 (/home/sanpi/sources/relm/relm-derive) [5/74] warning: large size difference between variants --> src/gen/parser.rs:88:5 | 88 | Return(Expr, Expr), | ^^^^^^^^^^^^^^^^^^ this variant is 560 bytes | = note: `#[warn(clippy::large_enum_variant)]` on by default note: and the second-largest variant is 280 bytes: --> src/gen/parser.rs:87:5 | 87 | CallReturn(Expr), | ^^^^^^^^^^^^^^^^ help: consider boxing the large fields to reduce the total size of the enum --> src/gen/parser.rs:88:5 | 88 | Return(Expr, Expr), | ^^^^^^^^^^^^^^^^^^ = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant warning: this function has too many arguments (8/7) --> src/gen/parser.rs:137:5 | 137 | / fn new_gtk(widget: GtkWidget, typ: Path, init_parameters: Vec, children: Vec, 138 | | properties: HashMap, child_properties: ChildProperties, child_events: ChildEvents, 139 | | nested_views: HashMap) -> Self | |_____________________________________________________^ | = note: `#[warn(clippy::too_many_arguments)]` on by default = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments warning: this function has too many arguments (8/7) --> src/gen/parser.rs:160:5 | 160 | / fn new_relm(widget: RelmWidget, typ: Path, init_parameters: Vec, children: Vec, 161 | | properties: HashMap, child_properties: ChildProperties, child_events: ChildEvents, 162 | | nested_views: HashMap) -> Self | |_____________________________________________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments warning: very complex type used. Consider factoring parts into `type` definitions --> src/gen/generator.rs:65:70 | 65 | ...driver: &mut Driver) -> (TokenStream, HashMap, HashMap, HashSet, TokenStream) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(clippy::type_complexity)]` on by default = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity warning: 4 warnings emitted Finished dev [unoptimized + debuginfo] target(s) in 0.50s ```

I don’t know how fix that.

antoyo commented 3 years ago

For the first warning, I guess it would be safer to Box the Expr in the variant Return to avoid a stack overflow in some cases.

For the functions with too many parameters, I'd just ignore them, as these are initializing structs with many fields anyway, so adding #[allow(clippy::too_many_arguments)] should do.

For the last error (src/gen/generator.rs:65:70), just make the return type be a struct instead of a tuple.

Also, it would be nice to run clippy somewhere in the CI.

Thanks for looking into this!

sanpii commented 3 years ago

For the first warning, I guess it would be safer to Box the Expr in the variant Return to avoid a stack overflow in some cases.

Done, but destructuring Box isn’t easy as a tuple.

For the functions with too many parameters, I'd just ignore them, as these are initializing structs with many fields anyway, so adding #[allow(clippy::too_many_arguments)] should do.

Ok, done.

For the last error (src/gen/generator.rs:65:70), just make the return type be a struct instead of a tuple.

I just created a type to limit API breaking. Why the gen function is public? Public for crate is not enough? If you are ok, I can write another PR to restrict function visibility and change its return struct.

Also, it would be nice to run clippy somewhere in the CI.

Done.

antoyo commented 3 years ago

Yep, the visibility can be crate.

sanpii commented 3 years ago

Ready to merge?

Yes, it’s ok for me.

sanpii commented 3 years ago

I just created a type to limit API breaking. Why the gen function is public? Public for crate is not enough? If you are ok, I can write another PR to restrict function visibility and change its return struct.

Actually pub items is not really public (proc-macro lib forbids pub items other than proc macro). So I created a struct type for this return type.