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

feat: read config toml file #7

Merged
merged 9 commits into from Jan 3, 2023

Conversation

1garo
Copy link
Contributor

@1garo 1garo commented Oct 11, 2022

This PR idea is to read rio's configuration file (written in .toml), parse it and returns Config struct

// initial idea for Config
pub struct Config {
    performance: Performances,
    width: Option<u16>,
    height: Option<u16>,
}
  • read toml file
  • parse toml file
  • get path based on OS
  • more configs???

config/src/lib.rs Outdated Show resolved Hide resolved
@raphamorim
Copy link
Owner

Hey @1garo , is okay to use serde 👍 Keep the good work! 🚀

#[derive(Debug, Deserialize, PartialEq)]
pub struct Config {
performance: Performances,
width: Option<u16>,
Copy link
Owner

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

Copy link
Contributor Author

@1garo 1garo Oct 13, 2022

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

Copy link
Owner

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 👍

Copy link
Contributor Author

@1garo 1garo Oct 14, 2022

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!

Copy link
Owner

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.

Copy link
Contributor Author

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

@1garo
Copy link
Contributor Author

1garo commented Oct 13, 2022

I feel that create the file to test is not the best way tho, I was thinking in load receive a &str with the path name or a buffer, but then would be bad for the usage, as we wan't the user just to Config::load(), tell me what you think.

@raphamorim
Copy link
Owner

I feel that create the file to test is not the best way tho, I was thinking in load receive a &str with the path name or a buffer, but then would be bad for the usage, as we wan't the user just to Config::new(), tell me what you think.

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

@raphamorim raphamorim merged commit 8fc4786 into raphamorim:main Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants