-
-
Notifications
You must be signed in to change notification settings - Fork 159
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
feat: read config toml file #7
Conversation
Hey @1garo , is okay to use serde 👍 Keep the good work! 🚀 |
config/src/lib.rs
Outdated
#[derive(Debug, Deserialize, PartialEq)] | ||
pub struct Config { | ||
performance: Performances, | ||
width: Option<u16>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Believe is better have as u16 instead of Option. To avoid additional unwrap when using the configuration struct later. Since all properties should have a default we can unwrap it on the new.
Also will be good to have a default in the impl. E.g: https://doc.rust-lang.org/std/default/trait.Default.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have started the implementation here
I couldn't come with a solution for when the user
have the config file, but didn't pass one of the parameters.
Because if I set the struct as below:
struct Config {
performance: Option<Performance>,
width: u16,
height: u16,
}
Now the problem is that when we parse, as the user
did not specified performance
, it is None
,
Config {
performance: None,
width: 300,
height: 200,
}
Did you have something in mind?
obs: the code can be not that rusty, still learning some patterns, if you think something can improve, just let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries at all @1garo, you are doing a great job. Usually when you're wrapping and come as None, you can use unwrap_or_else(||
to return the default. Let me know if you still have questions, we can book a day and do this part together 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds great, if you can talk today or tomorrow, just say where I can reach out to you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have telegram, just drop me a message on same username as GH. I also have discord just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think discord is easier for me, I used telegram once in my life. 1garo#7086
I feel that create the file to test is not the best way tho, I was thinking in |
For me both works, you can also write a helper function on tests to write a configuration file in memory and returns the file path. For example: fn create_temporary_config(width: u16, height: u16) -> std::ffi::OsString {
// create config in memory by serializing data
// save on a /tmp/test-config.toml or any other place
// feel free to implement the write however you want to 🚀
Config::load_from_path(filepath) // load_from_path should just call load() with a custom path
} Could be useful: https://stackoverflow.com/questions/71371861/how-do-i-write-an-array-of-homogenous-structs-in-rust-to-a-file-that-i-can-fread , https://betterprogramming.pub/reading-and-writing-a-file-in-rust-47d2bc7086ac |
This PR idea is to read rio's configuration file (written in
.toml
), parse it and returnsConfig
struct