4 ms·

Ask HN: Rust experts – please advise

3 points by gkuk 8y ago

Hi HackerNews community, This is my first post here so please forgive me if below looks silly. A few weeks back I have written a tool in rust. I am on holiday now and have some time to add improvements. Since I'm not expert in this language I'd love to get some feedback on where I can improve the code to make it more idiomatic and generally better. I use this tool on my servers to filter away all logs I know can be ignored so I can focus on logs that potentially require my attention. https://github.com/grzegorzk/logmap

vectorEQ 8y ago
Not being a rust programmer myself, my comment should be taken as that: You commented in your code on some long functions needing to be decomposed, that might be a good start, your own todo. :-) Other than this as a c programmer i see some opportunity in the _find_best_matching_filter_index function to perhaps apply a macro to the repeating syntax of let words = tst_utils::_words_vector_from_string("aaa bbb ccc ddd"); assert_eq!(log_filters._find_best_matching_filter_index(&words), 0); Where in C i would create something so i could type MACRO_NAME("aaa bbb ccc ddd") for example to make that part read more easily and make it easier to add or remove items there. I see macros are supported, but am because of my lack of rust knowledge unaware if my suggestion is possible at all. https://doc.rust-lang.org/book/ch19-06-macros.html https://doc.rust-lang.org/book/ch19-06-macros.html might shed some light on that. It's not really changing code but would make it much more readable. (macros are great for such things in C for sure if not overdone)
- gkuk 8y ago
  Thanks! I definitely need to be more DRY in tests.
  - vectorEQ 8y ago
    I always feel the same. usually only after i wrote something a million times i'll notice i could have made a macro and end up replacing it all again :D
- steveklabnik 8y ago
  You'd write a function to do this in Rust, not a macro.
steveklabnik 8y ago
A few quick things. Posting to users.rust-lang.org will probably get you more comments than here. Did you run clippy on this code? Rustfmt? those are two easy things. #![allow(dead_code)] is pretty suspicious... I would use format! here https://github.com/grzegorzk/logmap/blob/master/src/logmap.rs#L50-L59 https://github.com/grzegorzk/logmap/blob/master/src/logmap.r... https://github.com/grzegorzk/logmap/blob/master/src/logmap.rs#L90-L93 https://github.com/grzegorzk/logmap/blob/master/src/logmap.r... could be unwrap_or_else https://github.com/grzegorzk/logmap/blob/master/src/logmap.rs#L101 https://github.com/grzegorzk/logmap/blob/master/src/logmap.r... should just be `log_filters`, no need for the return; naming functions with `_` like https://github.com/grzegorzk/logmap/blob/master/src/logmap.rs#L104 https://github.com/grzegorzk/logmap/blob/master/src/logmap.r... is not idiomatic. It's already not public, no need for `_`. lines like https://github.com/grzegorzk/logmap/blob/master/src/logmap.rs#L117-L118 https://github.com/grzegorzk/logmap/blob/master/src/logmap.r... should be one line, no need for the type ascription there. the package is already called logmap, so https://github.com/grzegorzk/logmap/blob/master/src/lib.rs https://github.com/grzegorzk/logmap/blob/master/src/lib.rs is just redundant and adds an extra layer, that is, `logmap::logmap::foo` rather than just `logmap::foo`. This isn't an idiom issue, but you might like structopt over getopts.
- gkuk 8y ago
  Oh why why... was I not aware of cargo-clippy before...
- gkuk 8y ago
  Many thanks for your great feedback Steve! I had some issues with running unit tests and at the time I figured if I add lib.rs I can make my unit tests to run. I did not like logmap::logmap::foo and definitely need to revisit that.

3 points by gkuk 8y ago

vectorEQ 8y ago

Not being a rust programmer myself, my comment should be taken as that: You commented in your code on some long functions needing to be decomposed, that might be a good start, your own todo. :-) Other than this as a c programmer i see some opportunity in the _find_best_matching_filter_index function to perhaps apply a macro to the repeating syntax of let words = tst_utils::_words_vector_from_string("aaa bbb ccc ddd"); assert_eq!(log_filters._find_best_matching_filter_index(&words), 0); Where in C i would create something so i could type MACRO_NAME("aaa bbb ccc ddd") for example to make that part read more easily and make it easier to add or remove items there. I see macros are supported, but am because of my lack of rust knowledge unaware if my suggestion is possible at all. https://doc.rust-lang.org/book/ch19-06-macros.html https://doc.rust-lang.org/book/ch19-06-macros.html might shed some light on that. It's not really changing code but would make it much more readable. (macros are great for such things in C for sure if not overdone)

gkuk 8y ago
Thanks! I definitely need to be more DRY in tests.
- vectorEQ 8y ago
  I always feel the same. usually only after i wrote something a million times i'll notice i could have made a macro and end up replacing it all again :D
steveklabnik 8y ago
You'd write a function to do this in Rust, not a macro.

steveklabnik 8y ago

A few quick things. Posting to users.rust-lang.org will probably get you more comments than here. Did you run clippy on this code? Rustfmt? those are two easy things. #![allow(dead_code)] is pretty suspicious... I would use format! here https://github.com/grzegorzk/logmap/blob/master/src/logmap.rs#L50-L59 https://github.com/grzegorzk/logmap/blob/master/src/logmap.r... https://github.com/grzegorzk/logmap/blob/master/src/logmap.rs#L90-L93 https://github.com/grzegorzk/logmap/blob/master/src/logmap.r... could be unwrap_or_else https://github.com/grzegorzk/logmap/blob/master/src/logmap.rs#L101 https://github.com/grzegorzk/logmap/blob/master/src/logmap.r... should just be `log_filters`, no need for the return; naming functions with `_` like https://github.com/grzegorzk/logmap/blob/master/src/logmap.rs#L104 https://github.com/grzegorzk/logmap/blob/master/src/logmap.r... is not idiomatic. It's already not public, no need for `_`. lines like https://github.com/grzegorzk/logmap/blob/master/src/logmap.rs#L117-L118 https://github.com/grzegorzk/logmap/blob/master/src/logmap.r... should be one line, no need for the type ascription there. the package is already called logmap, so https://github.com/grzegorzk/logmap/blob/master/src/lib.rs https://github.com/grzegorzk/logmap/blob/master/src/lib.rs is just redundant and adds an extra layer, that is, `logmap::logmap::foo` rather than just `logmap::foo`. This isn't an idiom issue, but you might like structopt over getopts.

gkuk 8y ago
Oh why why... was I not aware of cargo-clippy before...
gkuk 8y ago
Many thanks for your great feedback Steve! I had some issues with running unit tests and at the time I figured if I add lib.rs I can make my unit tests to run. I did not like logmap::logmap::foo and definitely need to revisit that.