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

taha · 2025-04-11T19:20:25Z

taha commented

2025-04-11 19:20:25 +00:00

No description provided.

johnride requested changes 2025-04-11 19:32:05 +00:00

johnride left a comment

Code is simple, which is good enough for a mini prototype like this.

As it evolves it will require some kind of architecture to stay clean and be extensible.

At the moment a few things are required for this PR :

Add an end to end test somehow that will test a happy path, something that will run something like cargo run -p example-cli -- --run_score SuccessScore
Refactor the for loop as explained in the comment
Add the documentation to the README or something like that. The output of --help I guess? I guess clap provides this automatically?

Code is simple, which is good enough for a mini prototype like this. As it evolves it will require some kind of architecture to stay clean and be extensible. At the moment a few things are required for this PR : 1. Add an end to end test somehow that will test a happy path, something that will run something like `cargo run -p example-cli -- --run_score SuccessScore` 2. Refactor the for loop as explained in the comment 3. Add the documentation to the README or something like that. The output of --help I guess? I guess clap provides this automatically?

👍 1

harmony_cli/src/lib.rs Outdated

						
				@@ -0,0 +65,4 @@

				    } else {

				        let mut count = 0;

				        let mut found = false;

				        for s in scores_read_vec {

johnride commented

2025-04-11 19:23:59 +00:00

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"), } ```

taha marked this conversation as resolved

taha changed title from ~~harmony-cli initial PR~~ to feat: harmony-cli v0.1 #8

2025-04-11 20:41:37 +00:00

johnride requested changes 2025-04-12 12:09:51 +00:00

README.md Outdated

						
				@@ -11,0 +23,4 @@

				  -a, --all              Run all or nth

				  -n, --number <NUMBER>  Run nth matching, zero indexed [default: 0]

				  -h, --help             Print help

				  -V, --version          Print version

johnride commented

2025-04-12 12:09:48 +00:00

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.

johnride requested changes 2025-04-14 19:04:48 +00:00

johnride left a comment

cargo run -p example-cli -- -a

Passer l'option -a ne fait rien quand -r n'est pas specifie.

Je m'attendais a ce que -a tout seul roule tous les scores enregistres. Ca marche avec -r e -a

Il y a plein de clone_box() qui ne servent a rien. Pourquoi?
Les scores ne sont pas lances pour vrai. On voit le log "Running: SuccessScore" mais il n'est pas vraiment execute.

Ca doit etre execute pour vrai
Ca prend un test qui teste que c'est execute pour vrai. Tu pourrais creer un CreateFileScore ou quelque chose comme ca et verifier que le fichier est cree pour vrai genre dans /tmp/harmony-test-file-[randomsha1]

1. `cargo run -p example-cli -- -a ` Passer l'option -a ne fait rien quand -r n'est pas specifie. Je m'attendais a ce que -a tout seul roule tous les scores enregistres. Ca marche avec `-r e -a` 2. Il y a plein de `clone_box()` qui ne servent a rien. Pourquoi? 3. Les scores ne sont pas lances pour vrai. On voit le log "Running: SuccessScore" mais il n'est pas vraiment execute. - Ca doit etre execute pour vrai - Ca prend un test qui teste que c'est execute pour vrai. Tu pourrais creer un CreateFileScore ou quelque chose comme ca et verifier que le fichier est cree pour vrai genre dans `/tmp/harmony-test-file-[randomsha1]`

taha force-pushed harmony-cli from 559339f2eb to ee9270d771

2025-04-16 17:41:16 +00:00

Compare

johnride requested changes 2025-04-17 15:24:57 +00:00

johnride left a comment

Few things to fix still :

Tests belong in the harmony_tui crate. Tests live closes to the code they are actually testing. If iyou want to add tests to the example project, they should test behavior specific to the example project. In the current case, harmony_tui exposes functionnality, this functionnality must be tested within the module.
running with -a should run all scores, as of now it does nothing
Having to specify -r is annoying, but a minor annoyance. I think a better UX would be to replace -r with -y --yes that runs without prompting. When -y is not specified, prompt with something like : Do you want to run : [ List of scores matching whatever arguments are passed ] Y/n :
Specifying -n does not filter nth correctly.

../../target/debug/example-cli -n 1 -r                                                                                                                                                              101 ✘
Running: SuccessScore
Running: ErrorScore

thread 'main' panicked at examples/cli/src/main.rs:19:38:
called `Result::unwrap()` on an `Err` value: InterpretError { msg: "Error Score default error" }

-l should show the number matching the -n argument for better UX :

 ../../target/debug/example-cli -n 1 -r -l                                                                                                                                                           101 ✘
Available scores (use -n <nth> to run it):
1. SuccessScore,
2. ErrorScore,
3. PanicScore,

5.1. We could also just remove the -n option for now to simplify the PR

Few things to fix still : 1. Tests belong in the harmony_tui crate. Tests live closes to the code they are actually testing. If iyou want to add tests to the example project, they should test behavior specific to the example project. In the current case, harmony_tui exposes functionnality, this functionnality must be tested within the module. 2. running with -a should run all scores, as of now it does nothing 3. Having to specify -r is annoying, but a minor annoyance. I think a better UX would be to replace -r with `-y --yes` that runs without prompting. When `-y` is not specified, prompt with something like : `Do you want to run : [ List of scores matching whatever arguments are passed ] Y/n :` 4. Specifying -n does not filter nth correctly. ``` ../../target/debug/example-cli -n 1 -r  101 ✘ Running: SuccessScore Running: ErrorScore thread 'main' panicked at examples/cli/src/main.rs:19:38: called `Result::unwrap()` on an `Err` value: InterpretError { msg: "Error Score default error" } ``` 5. -l should show the number matching the -n argument for better UX : ``` ../../target/debug/example-cli -n 1 -r -l  101 ✘ Available scores (use -n <nth> to run it): 1. SuccessScore, 2. ErrorScore, 3. PanicScore, ``` 5.1. We could also just remove the -n option for now to simplify the PR

taha added 1 commit 2025-04-17 19:22:05 +00:00

Add prompt on run if --yes not specified, add tests, separate out function for filtering, bug fixes, etc b781a7ff4a

taha added 1 commit 2025-04-17 22:04:34 +00:00

Update readme, add conflict f75d03c570

taha added 1 commit 2025-04-17 22:22:57 +00:00

add index to display 92e60b00d2

taha added 1 commit 2025-04-17 22:26:00 +00:00

Change help for --yes ff568cd1b9

taha added 2 commits 2025-04-17 22:39:32 +00:00

add conflicts for options with interactive b7fcc66fa5

remove conflict with all and list 9276ec05f0

taha added 1 commit 2025-04-18 13:59:11 +00:00

rset harmony dir to master d92c35987a

taha added 1 commit 2025-04-18 14:01:40 +00:00

remove unrelated 8a1e20bda5

taha added 1 commit 2025-04-18 14:05:23 +00:00

Fix compilation errors 2d97cac64b

johnride approved these changes 2025-04-18 14:57:26 +00:00

johnride left a comment

This is just so much better. And it shows in the UX. Usually a good UX requires good code and good code produces a good UX.

Thanks for the nice work.

Some minor comments but they can all be fixed in a subsequent PR. Comments about comments are quite important though, either fix them here or in another pr, they're just smelly right now.

This is just so much better. And it shows in the UX. Usually a good UX requires good code and good code produces a good UX. Thanks for the nice work. Some minor comments but they can all be fixed in a subsequent PR. Comments about comments are quite important though, either fix them here or in another pr, they're just smelly right now.

harmony_cli/src/lib.rs

						
				@@ -0,0 +50,4 @@

				) -> Vec<Box<dyn Score<T>>> {

				    let scores = maestro.scores();

				    let scores_read = scores.read().expect("Should be able to read scores");

				    let mut scores_vec: Vec<Box<dyn Score<T>>> = match filter {

johnride commented

2025-04-18 14:34:24 +00:00

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.

harmony_cli/src/lib.rs Outdated

						
				@@ -0,0 +70,4 @@

				    return scores_vec;

				}

				fn list_scores_with_index<T: Topology + std::fmt::Debug + Send + Sync + 'static>(

johnride commented

2025-04-18 14:36:00 +00:00

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

harmony_cli/src/lib.rs Outdated

						
				@@ -0,0 +113,4 @@

				        return Ok(());

				    }

				    // if no score to run provided, and list isn't specified, print help

johnride commented

2025-04-18 14:48:08 +00:00

Either I don't understand it or it's an outdated comment, I think those types of comment explaining WHAT the code is doing are generally bad. Comments should explain WHY. If you need to express a WHAT, use a function.

https://news.ycombinator.com/item?id=8073620

Either I don't understand it or it's an outdated comment, I think those types of comment explaining WHAT the code is doing are generally bad. Comments should explain WHY. If you need to express a WHAT, use a function. https://news.ycombinator.com/item?id=8073620

harmony_cli/src/lib.rs Outdated

						
				@@ -0,0 +131,4 @@

				        }

				    }

				    // if all is specified, run all. Otherwise, run the first match or specified index

johnride commented

2025-04-18 14:46:16 +00:00

Weird comment here again, does not relate directly to the code block. I think I'd rather not have it.

harmony_cli/src/lib.rs

						
				@@ -0,0 +308,4 @@

				            Box::new(SuccessScore {}),

				            Box::new(ErrorScore {}),

				            Box::new(PanicScore {}),

				        ]);

johnride commented

2025-04-18 14:27:07 +00:00

Those 8 lines should be extracted to a utilty function in the test module such as "init_test_maestro() -> Maestro"

taha added 3 commits 2025-04-19 01:13:01 +00:00

extracted test function 09e61eaabc

fix comments 122c75f4be

add TODO 8c7f7471b2

taha merged commit abd20b96a2 into master

2025-04-19 01:13:40 +00:00

taha deleted branch harmony-cli

2025-04-19 01:13:40 +00:00

taha referenced this issue from a commit

2025-04-19 01:13:41 +00:00

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

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: NationTech/harmony#9