Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve code readability via match expression #248

Merged
merged 4 commits into from Jun 6, 2023

Conversation

LiChenG-P
Copy link
Contributor

I'm new to Rust and I came across some code that could be enhanced with pattern matching. There are no logic changes, and it compiles without any issues. If you notice any mistakes, please inform me, as I'm making a sincere effort to become familiar with Rust. Thank you for your valuable time!

@GyulyVGC GyulyVGC added the enhancement New feature, request, or improvement label Jun 6, 2023
@GyulyVGC
Copy link
Owner

GyulyVGC commented Jun 6, 2023

Hi @LiChenG-P

Thank you for the PR.
I'm happy you are making your first Rust contributions!
I will check your PR and let you know.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jun 6, 2023

I've just run cargo fmt.
You can run this command to format the code according to the best practices, if you didn't know about it.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jun 6, 2023

Another useful command you may be interested in is cargo clippy.
It gives you any type of warning about what could be improved in the code.

Running the pedantic version of clippy (cargo clippy -- -W clippy::pedantic) on your code gave me these warnings:

warning: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
  --> src/main.rs:69:27
   |
69 |       let config_settings = match confy::load::<ConfigSettings>("sniffnet", "settings") {
   |  ___________________________^
70 | |         Ok(setting) => setting,
71 | |         Err(_) => {
72 | |             confy::store("sniffnet", "settings", ConfigSettings::default()).unwrap_or(());
73 | |             ConfigSettings::default()
74 | |         }
75 | |     };
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else
   = note: `-W clippy::single-match-else` implied by `-W clippy::pedantic`
help: try this
   |
69 ~     let config_settings = if let Ok(setting) = confy::load::<ConfigSettings>("sniffnet", "settings") { setting } else {
70 +         confy::store("sniffnet", "settings", ConfigSettings::default()).unwrap_or(());
71 +         ConfigSettings::default()
72 ~     };
   |

warning: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
  --> src/main.rs:77:25
   |
77 |       let config_device = match confy::load::<ConfigDevice>("sniffnet", "device") {
   |  _________________________^
78 | |         Ok(device) => device,
79 | |         Err(_) => {
80 | |             confy::store("sniffnet", "device", ConfigDevice::default()).unwrap_or(());
81 | |             ConfigDevice::default()
82 | |         }
83 | |     };
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#single_match_else
help: try this
   |
77 ~     let config_device = if let Ok(device) = confy::load::<ConfigDevice>("sniffnet", "device") { device } else {
78 +         confy::store("sniffnet", "device", ConfigDevice::default()).unwrap_or(());
79 +         ConfigDevice::default()
80 ~     };
   |

Which I have fixed with my last commit.

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jun 6, 2023

Overall you did a good job, especially for what concerns the keyboard shortcut section: it's now a lot more readable.

Another interesting hint I can give you is that the commands I mentioned above are useful also to run automated checks which are triggered by GitHub at every commit or pull request.
Check this file, if you want to see how the Rust workflow is built for Sniffnet.

If you have any other observation or comment let me know.
If everything's ok for you, I'll merge your PR!

@LiChenG-P
Copy link
Contributor Author

Thanks for the tips! I've been using CLion and it automatically applies some mild formatting whenever I save the code. I initially thought it was cargo fmt, but apparently that's not the case 😰. However, I wasn't aware of the cargo clippy feature. It's definitely a valuable lesson learned!

@LiChenG-P
Copy link
Contributor Author

So what's left to do? Everything seems fine to me😄. Thanks again for your helpful response!

@GyulyVGC
Copy link
Owner

GyulyVGC commented Jun 6, 2023

So what's left to do? Everything seems fine to me😄. Thanks again for your helpful response!

Let's merge then!
Thanks for your contribution!

@GyulyVGC GyulyVGC merged commit 76c6f8c into GyulyVGC:main Jun 6, 2023
3 checks passed
@GyulyVGC
Copy link
Owner

GyulyVGC commented Jun 6, 2023

@all-contributors please add @LiChenG-P for code.

@allcontributors
Copy link
Contributor

@GyulyVGC

I've put up a pull request to add @LiChenG-P! 🎉

@GyulyVGC GyulyVGC added this to the v1.2.1 milestone Jun 6, 2023
@GyulyVGC GyulyVGC mentioned this pull request Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, request, or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants