NationTech/harmony
5
0
Fork 2
Code Issues 37 Pull Requests 16 Actions Packages Projects Releases 1 Wiki Activity

feat: harmony-cli v0.1 #8 #9

Merged
taha merged 23 commits from harmony-cli into master 2025-04-19 01:13:40 +00:00
Conversation 5 Commits 23 Files Changed 8 +337 -4
7 changed files with 337 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit e845544035 - Show all commits

12
README.md
View File

@@ -14,13 +14,14 @@ This is the harmony CLI, a minimal implementation
The current help text:
```
Usage: example-cli [OPTIONS]
````
Usage: harmony_cli [OPTIONS]
Options:
-r, --run <RUN> Name of score to run
-i, --interactive Run interactive or not
-a, --all Run all or nth
-r, --run Run score(s) or not
-f, --filter <FILTER> Filter query
-i, --interactive Run interactive TUI or not
-a, --all Run all or nth, defaults to all
-n, --number <NUMBER> Run nth matching, zero indexed [default: 0]
-l, --list list scores, will also be affected by run filter
johnride commented 2025-04-12 12:09:48 +00:00
Outdated
Review
Copy Link

Je viens de l'essayer, semble pas pire, mais je pense que ca prend quelques trucs pour que le UX soit correct :

  • Une option --list qui liste les scores
  • La feature tui devrait etre la par defaut
  • Le comportement de nth me semble etrange. Pas sur qu'on veut prendre le nth apres le filtrage.
  • Lorsqu'on ne passe aucun argument on devrait voir soit le help ou la liste des scores. Probablement les deux en fait.
Je viens de l'essayer, semble pas pire, mais je pense que ca prend quelques trucs pour que le UX soit correct : - Une option --list qui liste les scores - La feature tui devrait etre la par defaut - Le comportement de nth me semble etrange. Pas sur qu'on veut prendre le nth apres le filtrage. - Lorsqu'on ne passe aucun argument on devrait voir soit le help ou la liste des scores. Probablement les deux en fait.
-h, --help Print help
@@ -30,3 +31,4 @@ Options:
## Core architecture
![Harmony Core Architecture](docs/diagrams/Harmony_Core_Architecture.drawio.svg)
````

49
harmony_cli/src/lib.rs
View File

@@ -1,3 +1,5 @@
use std::error::Error;
use clap::builder::ArgPredicate;
use clap::{Arg, CommandFactory, Parser};
use harmony;
@@ -9,26 +11,20 @@ use harmony_tui;
#[derive(Parser, Debug)]
#[command(version, about, long_about = None)]
struct Args {
/// Name of score to run
#[cfg(feature = "tui")]
#[arg(
short,
long,
default_value_if("interactive", ArgPredicate::IsPresent, Some(""))
)]
run: Option<String>,
/// Run score(s) or not
#[arg(short, long, default_value_t = false)]
run: bool,
/// Name of score to run
#[cfg(not(feature = "tui"))]
/// Filter query
#[arg(short, long)]
run: String,
filter: Option<String>,
/// Run interactive or not
/// Run interactive TUI or not
#[arg(short, long, default_value_t = false)]
interactive: bool,
/// Run all or nth
#[arg(short, long, default_value_t = false)]
/// Run all or nth, defaults to all
#[arg(short, long, default_value_t = true)]
all: bool,
/// Run nth matching, zero indexed
@@ -56,41 +52,46 @@ pub async fn init<T: Topology + std::fmt::Debug + Send + Sync + 'static>(
}
johnride commented 2025-04-18 14:34:24 +00:00
Review
Copy Link

So much better. I think there are more Rust idiomatic ways to handle that with Slices or something but it's good enough :

  • Clear, easy to read and understand
  • Relatively flat, not too much nested ifs and fors
  • Leveraging functional Rust, usually makes for more robust code easily

Now though you should at least reverse the map and filter calls so scores don't all get cloned for nothing.

In general you always filter your ensemble before processing.

So much better. I think there are more Rust idiomatic ways to handle that with Slices or something but it's good enough : - Clear, easy to read and understand - Relatively flat, not too much nested ifs and fors - Leveraging functional Rust, usually makes for more robust code easily Now though you should at least reverse the map and filter calls so scores don't all get cloned for nothing. In general you always filter your ensemble before processing.
// if no score to run provided, and list isn't specified, print help
if args.run.is_none() && !args.list {
if !args.run && !args.list {
println!("Please include at least --run or --list");
Args::command().print_help();
return Ok(());
}
let scores = maestro.scores();
let scores_read = scores.read().expect("Should be able to read scores");
let scores_vec: Vec<Box<dyn Score<T>>> = match args.run.clone() {
Some(filter) => scores_read
let scores_vec: Vec<Box<dyn Score<T>>> = match args.filter.clone() {
Some(f) => scores_read
.iter()
.map(|s| s.clone_box())
.filter(|s| s.name().contains(&filter))
.filter(|s| s.name().contains(&f))
.collect(),
taha marked this conversation as resolved Outdated
johnride commented 2025-04-11 19:23:59 +00:00
Outdated
Review
Copy Link

Calling a function here that uses .iter().filter() and returns an option is more idiomatic rust (and more robust) than the mut bool found and mut count Just filter the array and them you can match filtered.len()

let filtered = scores_read_vec.iter().filter(...).collect();
match filtered.len() { // Not sure about .len, could be length or size
  0 => todo!("Handle zero"),
  1 => todo!("Handle one"),
  _ => todo!("Handle more than one"),
}
Calling a function here that uses .iter().filter() and returns an option is more idiomatic rust (and more robust) than the `mut bool found` and `mut count` Just filter the array and them you can match `filtered.len()` ```rust let filtered = scores_read_vec.iter().filter(...).collect(); match filtered.len() { // Not sure about .len, could be length or size 0 => todo!("Handle zero"), 1 => todo!("Handle one"), _ => todo!("Handle more than one"), } ```
None => scores_read.iter().map(|s| s.clone_box()).collect(),
};
if scores_vec.len() == 0 {
return Err("No score found".into());
johnride commented 2025-04-18 14:36:00 +00:00
Outdated
Review
Copy Link

This function, though pretty simple, deserves a test. Could be a good place for a doctest. Not necessary though, just spilling my thoughts.

This function, though pretty simple, deserves a test. Could be a good place for a doctest. Not necessary though, just spilling my thoughts.
}
// if list option is specified, print filtered list and exit
if args.list {
println!("Available scores: {:#?}", scores_vec);
return Ok(());
}
if scores_vec.len() == 0 {
return Err("No score containing query found".into());
}
// if all is specified, run all. Otherwise, run the first match or specified index
if args.all {
for s in scores_vec {
println!("Running: {}", s.clone_box().name());
println!("Running: {}", s.name());
maestro.interpret(s).await?;
}
} else {
let score = scores_vec.get(args.number as usize);
match score {
Some(s) => println!("Running: {}", s.clone_box().name()),
Some(s) => {
println!("Running: {}", s.name());
maestro.interpret(s.clone_box()).await?;
}
None => return Err("Invalid index given".into()),
}
}