start working on a cli app #7

Merged
schrottkatze merged 12 commits from :app into main 2024-01-20 19:09:52 +00:00
Owner

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 11 commits 2024-01-11 15:58:55 +00:00
schrottkatze added 1 commit 2024-01-12 08:32:02 +00:00
schrottkatze requested review from multisamplednight 2024-01-12 08:32:31 +00:00
schrottkatze added 1 commit 2024-01-12 08:36:37 +00:00
multisamplednight reviewed 2024-01-12 15:08:12 +00:00
Cargo.toml Outdated
@ -1,5 +1,5 @@
[workspace]
members = [
members = [ "crates/app",
Collaborator

Probably unintentional formatting.

Probably unintentional formatting.
Author
Owner

that was automatically modified to be like that by cargo

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"
Collaborator

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

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

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
@ -0,0 +41,4 @@
json_path
} else {
eprintln!("{}: couldn't find config file", "Fatal error".red());
process::exit(1)
Collaborator

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

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

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

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
@ -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) -> ! {
Collaborator

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`.
Author
Owner

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

ye, that's a hack bc i was too lazy to implement a proper error type ^^'
multisamplednight marked this conversation as resolved
@ -0,0 +11,4 @@
fn main() {
let args = Args::parse();
let cfg = Configs::read(args.config_file);
Collaborator

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.
Author
Owner

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

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

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
@ -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());
Collaborator

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.
Author
Owner

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

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

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 added 1 commit 2024-01-12 21:19:57 +00:00
schrottkatze added 2 commits 2024-01-15 08:05:06 +00:00
Author
Owner

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?
schrottkatze added 1 commit 2024-01-15 09:43:45 +00:00
Collaborator

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
@ -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")]
Collaborator
Might want a [`clap::builder::BoolishValueParser`](https://docs.rs/clap/latest/clap/builder/struct.BoolishValueParser.html) here.
Author
Owner

How do I use that with claps derive api?

How do I use that with claps derive api?
Collaborator

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
@ -0,0 +11,4 @@
pub struct Configs {
#[serde(default = "default_example_value")]
pub example_value: i32,
#[serde(default = "default_no_startup_msg")]
Collaborator

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
schrottkatze added 1 commit 2024-01-19 07:54:43 +00:00
schrottkatze added 1 commit 2024-01-19 08:51:53 +00:00
schrottkatze added 1 commit 2024-01-19 17:50:56 +00:00
multisamplednight approved these changes 2024-01-20 17:27:11 +00:00
Collaborator

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

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
Author
Owner

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

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

Chances are you'll want to git remote update; git rebase origin/main; git push --force-with-lease after #2 is merged

Chances are you'll want to `git remote update; git rebase origin/main; git push --force-with-lease` after #2 is merged
schrottkatze force-pushed app from 32c981f245 to 746c81ab60 2024-01-20 19:05:12 +00:00 Compare
schrottkatze added 1 commit 2024-01-20 19:07:59 +00:00
multisamplednight approved these changes 2024-01-20 19:09:06 +00:00
schrottkatze merged commit 48458fd1c9 into main 2024-01-20 19:09:52 +00:00
schrottkatze deleted branch app 2024-01-20 19:09:52 +00:00
schrottkatze referenced this pull request from a commit 2024-01-23 12:55:02 +00:00
Sign in to join this conversation.
No reviewers
No labels
proposal
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: katzen-cafe/iowo#7
No description provided.