Next exercise #1822
Next exercise #1822
Conversation
Added a new property to Exercise, next_path. I populate this property, when loading the exercises.
There was a problem hiding this comment.
Hi,
thank you for helping to improve this project 👍.
A little disclaimer:
I am not a maintainer of this project, so I can't make any decisions here. These are just my personal observations and opinions.
I have added a few comments to your code with some thoughts that came to my mind while reading your PR.
I hope those comments don't sound harsh or unfriendly, they are just meant as some ideas that could be worth to consider.
While reading your PR it's a bit unclear to me, what problem you are trying to solve (this could also be an issue on my end 😉).
While I think it's a good idea to provide the students with some guidance through the exercises, I'm not sure if displaying the next path in the success message is providing much benefit over the current situation.
This is especially because in rustlings watch mode Rustlings tries to compile the next exercise automatically after you remove the I AM NOT DONE comment and displays the file in which the error for the next exercise occurs.
In VSCode this path is even clickable, which will open the exercise for you.
So maybe you can elaborate a bit more on why the current solution doesn't fit your needs and what should be improved.
Okay somehow I missed your "Waiting 1sec for rustlings watch is inconvenient" comment 🙈.
Nevertheless I think I would still prefer to "compute" the next exercise path on demand over changing the exercise schema and updating all exercises in advance.
| pub hint: String, | ||
| // The path of the next exercise | ||
| pub next_path: Option<PathBuf>, |
There was a problem hiding this comment.
Personally I think adding a separate field for the next exercise is unnecessary, since the exercises are in order in the info.toml.
Therefore the path to the next exercise can trivially be retrieved by getting the next exercise from the list and looking at the path field of that exercise.
I think we should not add multiple sources for the same information since this leads to confusion and can introduce additional errors if the two information sources diverge.
| fn edit_exercises(exercises: &mut Vec<Exercise>) { | ||
| let siz = exercises.len(); | ||
| for i in 0..siz { | ||
| if i == siz - 1 { | ||
| exercises[i].next_path = None; | ||
| break; | ||
| } | ||
| let next_path = &exercises[i + 1].path; | ||
| let mut x = PathBuf::new(); | ||
| x.push(next_path); | ||
| exercises[i].next_path = Some(x); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Please see my comment regarding the new field.
Additionally if we are computing the next_paths anyway we could also do this on demand and avoid the changing of the info.toml schema.
| match &exercise.next_path { | ||
| Some(path) => { | ||
| println!("Next up: {:?}", path); | ||
| } | ||
| None => {} | ||
| } | ||
|
|
There was a problem hiding this comment.
Not sure if this is the best location to print the next exercise, since after that there is a lot of output which belongs to the current exercise.
This might either confuse users a bit or the next exercise is overlooked since there is so much additional output following.
While doing the exercises I found myself wanting this small piece of information which file I'm going to have to edit next, so while the 1 sec timeout runs out in the rustlings watch, I can be already switched to that file.