start working on a cli app #7

schrottkatze · 2024-01-11T15:58:55Z

schrottkatze commented

2024-01-11 15:58:55 +00:00

I've started work on a cli app and some config file support stuff...

The end goal for this PR is, that this becomes the primary iOwO binary.

I've started work on a cli app and some config file support stuff... The end goal for this PR is, that this becomes the primary iOwO binary.

schrottkatze added 1 commit

2024-01-11 15:58:55 +00:00

actually write things 8ddd1748b8

requested review from multisamplednight

2024-01-12 08:32:31 +00:00

multisamplednight reviewed

2024-01-12 15:08:12 +00:00

Cargo.toml Outdated

					
				@ -1,5 +1,5 @@

				[workspace]

				members = [ 

				members = [ "crates/app",

multisamplednight commented

2024-01-12 15:07:46 +00:00

Probably unintentional formatting.

schrottkatze commented

2024-01-12 19:34:03 +00:00

that was automatically modified to be like that by cargo

multisamplednight marked this conversation as resolved

Cargo.toml Outdated

					
				@ -10,1 +10,4 @@

				clap = { version = "4", features = [ "derive" ] }

				serde = { version = "1.0", features = [ "derive" ] }

				ron = "0.8"

				serde_json = "1.0"

multisamplednight commented

2024-01-12 15:08:10 +00:00

Is there a reason ron and serde_json are included workspace-wide?

Is there a reason `ron` and `serde_json` are included workspace-wide?

schrottkatze commented

2024-01-12 19:35:08 +00:00

i wanted the version synced workspace wide, but ye, thinking about it the only place where they actually should get used is app

i wanted the version synced workspace wide, but ye, thinking about it the only place where they actually should get used is `app`

schrottkatze marked this conversation as resolved

crates/app/src/config.rs Outdated

					
				@ -0,0 +41,4 @@

				                    json_path

				                } else {

				                    eprintln!("{}: couldn't find config file", "Fatal error".red());

				                    process::exit(1)

multisamplednight commented

2024-01-12 15:07:04 +00:00

For later, not now: Should use proper error handling.

schrottkatze commented

2024-01-12 19:36:26 +00:00

put that on the non-existing todo list for when we have a proper error handling and reporting system

multisamplednight marked this conversation as resolved

crates/app/src/error_reporting.rs Outdated

					
				@ -0,0 +11,4 @@

				    report_serde_err(src, line, col, err.to_string())

				}

				pub fn report_serde_err(src: &str, line: usize, col: usize, msg: String) -> ! {

multisamplednight commented

2024-01-12 15:03:53 +00:00

Maybe report_serde_err could take a custom error, which the errors from ron and serde_json are converted into.

Then that streamlined error could report a bit more specific using serde_error::Error::classify and ron::Error.

Maybe `report_serde_err` could take a custom error, which the errors from `ron` and `serde_json` are converted into. Then that streamlined error could report a bit more specific using `serde_error::Error::classify` and `ron::Error`.

schrottkatze commented

2024-01-12 19:35:48 +00:00

ye, that's a hack bc i was too lazy to implement a proper error type ^^'

multisamplednight marked this conversation as resolved

crates/app/src/main.rs Outdated

					
				@ -0,0 +11,4 @@

				fn main() {

				    let args = Args::parse();

				    let cfg = Configs::read(args.config_file);

multisamplednight commented

2024-01-12 15:05:31 +00:00

Might want to streamline those two, in order to avoid accidentally checking only one of them in future.

The figment crate might be worth looking at in that case.

Might want to streamline those two, in order to avoid accidentally checking only one of them in future. The `figment` crate might be worth looking at in that case.

schrottkatze commented

2024-01-12 19:39:34 +00:00

i'd prefer to try at least to some degree reduce dependencies, but i'll work on something like that then

multisamplednight commented

2024-01-12 20:54:55 +00:00

You don't need to use figment. Custom merge logic works fine, too.

You don't _need_ to use `figment`. Custom merge logic works fine, too.

multisamplednight marked this conversation as resolved

crates/app/src/welcome_msg.rs Outdated

					
				@ -0,0 +2,4 @@

				pub fn print_startup_msg() {

				    // now or fallback to utc

				    let now = OffsetDateTime::now_local().unwrap_or_else(|_| OffsetDateTime::now_utc());

multisamplednight commented

2024-01-12 15:00:04 +00:00

Can always use now_utc, since no calculations are performed based on the timezone.

Can always use `now_utc`, since no calculations are performed based on the timezone.

schrottkatze commented

2024-01-12 19:37:23 +00:00

the day might very well be timezone dependent, depending on where you live though?

multisamplednight commented

2024-01-12 21:04:52 +00:00

Nevermind, I misinterpreted now_utc. I thought it'd return the current system time and assume it to be UTC (like SystemTime::now does), but it doesn't:

>> :dep time = { features = ["local-offset"] }
   Compiling libc v0.2.152
   Compiling powerfmt v0.2.0
   Compiling num_threads v0.1.6
   Compiling deranged v0.3.11
   Compiling time v0.3.31
>> use time::OffsetDateTime;
>> OffsetDateTime::now_utc()
2024-01-12 21:03:54.32211146 +00:00:00
>> OffsetDateTime::now_local()
Ok(2024-01-12 22:03:58.663618677 +01:00:00)

Nevermind, I misinterpreted `now_utc`. I thought it'd return the current system time and assume it to be UTC (like `SystemTime::now` does), but it doesn't: ``` >> :dep time = { features = ["local-offset"] } Compiling libc v0.2.152 Compiling powerfmt v0.2.0 Compiling num_threads v0.1.6 Compiling deranged v0.3.11 Compiling time v0.3.31 >> use time::OffsetDateTime; >> OffsetDateTime::now_utc() 2024-01-12 21:03:54.32211146 +00:00:00 >> OffsetDateTime::now_local() Ok(2024-01-12 22:03:58.663618677 +01:00:00) ```

multisamplednight marked this conversation as resolved

schrottkatze commented

2024-01-15 08:19:23 +00:00

A few questions to be answered:

If there's no config file, should we fall back to defaults? What if we have one, but reading it fails?

Also, what about invalid values? Or if it's empty?

A few questions to be answered: If there's no config file, should we fall back to defaults? What if we have one, but reading it fails? Also, what about invalid values? Or if it's empty?

multisamplednight commented

2024-01-18 17:48:08 +00:00

If there's no config file, should we fall back to defaults?

Yes, imo. Allows trying out iowo quickly without actually leaving permanent traces.

What if we have one, but reading it fails?
Also, what about invalid values?

Yield a warning and continue running. Chances are the user still wants their pipeline executed. Or perhaps the config file was written for a newer version, or a different installation.

Or if it's empty?

An empty config file is not an error, imo. Just assume defaults.

> If there's no config file, should we fall back to defaults? Yes, imo. Allows trying out iowo quickly without actually leaving permanent traces. > What if we have one, but reading it fails? > Also, what about invalid values? Yield a warning and continue running. Chances are the user still wants their pipeline executed. Or perhaps the config file was written for a newer version, or a different installation. > Or if it's empty? An empty config file is not an error, imo. Just assume defaults.

multisamplednight reviewed

2024-01-18 18:01:49 +00:00

crates/app/src/config/cli.rs Outdated

					
				@ -0,0 +7,4 @@

				    /// Read this config file.

				    #[arg(short, long)]

				    pub config_path: Option<PathBuf>,

				    #[arg(long, env = "NO_STARTUP_MESSAGE", default_value = "false")]

multisamplednight commented

2024-01-18 17:56:31 +00:00

Might want a clap::builder::BoolishValueParser here.

Might want a [`clap::builder::BoolishValueParser`](https://docs.rs/clap/latest/clap/builder/struct.BoolishValueParser.html) here.

schrottkatze commented

2024-01-19 07:18:25 +00:00

How do I use that with claps derive api?

multisamplednight commented

2024-01-19 11:00:17 +00:00

Through

-    #[arg(long, env = "NO_STARTUP_MESSAGE", default_value = "false")] 
+    #[arg(long, env = "NO_STARTUP_MESSAGE", action = ArgAction::SetTrue, value_parser = BoolishValueParser::new())]

Through ```diff - #[arg(long, env = "NO_STARTUP_MESSAGE", default_value = "false")] + #[arg(long, env = "NO_STARTUP_MESSAGE", action = ArgAction::SetTrue, value_parser = BoolishValueParser::new())] ```

schrottkatze marked this conversation as resolved

crates/app/src/config/config_file.rs Outdated

					
				@ -0,0 +11,4 @@

				pub struct Configs {

				    #[serde(default = "default_example_value")]

				    pub example_value: i32,

				    #[serde(default = "default_no_startup_msg")]

multisamplednight commented

2024-01-18 18:00:17 +00:00

Since the default is false which is also returned by `bool::default, the extra function is not necessary:

-    #[serde(default = "default_no_startup_msg")]
+    #[serde(default)]

If you do desire to keep the functions to allow more detailed behavior in future, I suggest renaming default_no_startup_msg to to just bool_false and creating a bool_true counterpart once necessary.

Since the default is `false` which is also returned by `bool::default, the extra function is not necessary: ```diff - #[serde(default = "default_no_startup_msg")] + #[serde(default)] ``` If you do desire to keep the functions to allow more detailed behavior in future, I suggest renaming `default_no_startup_msg` to to just `bool_false` and creating a `bool_true` counterpart once necessary.

schrottkatze marked this conversation as resolved

multisamplednight approved these changes

2024-01-20 17:27:11 +00:00

multisamplednight commented

2024-01-20 17:28:44 +00:00

Thank you!! Feel free to merge if you're finished with this.

schrottkatze changed title from ~~WIP: start working on a cli app~~ to start working on a cli app

2024-01-20 18:06:12 +00:00

schrottkatze commented

2024-01-20 18:06:58 +00:00

I think this one also depends on #2, so lets get that finally merged first :)