Rewrite bin/init_exercise.py in Rust #660

ZapAnton · 2018-09-18T09:38:56Z

The purpose of this PR is to replace the functionality of the existing bin/init_exersice.py script with the Rust crate exercise and to add additional functionality as per #641.

It is opened as In Progress in order for maintainers to track any undesirable decisions/functionality in the new crate, since it is a medium-complexity project, so disagreements may arise.

In addition to the future suggestions the current work plan looks like this:

Major Tasks

Reach an agreement on the new crate's API/commands
Implement configure subcommand
Implement update subcommand
Update the docs to mention the usage of the new crate

Minor Tasks

Add any missing comments/fields from the canonical-data.json to the generated test suite
Improve the crate's functions documentation
Prettify the generated test suite when the maplit crate is used. Currently it is an unreadable mess and rustfmt does not help
General refactoring. I am not the most prominent rustcean out there, so my solutions for the problem may not be the most optimal

Note that as per #641 (comment) the crate name is changed from init_exercise to exercise, so the prefix in the commit messages is changed from init_exercise: to exercise: accordingly

If this PR is merged, closes #641

…gure flag

…e_default_meta

… suite

…he process is finished running

ZapAnton · 2018-09-18T09:50:30Z

Questions regarding the update subcommand:

As I understand it, the desirable algorithm is to get the latest canonical-data.json for the exercise and to apply any diffs to the existing test suite:

If a new case/property is added to the canonical-data, add the appropriate functions to the existing test suite - no problems here
If the existing case is updated, add a new function to the test suite and do not touch the existing function. Question: How to track the updates to the cases? We cannot use the existing test suite, as the person who implements the exercise is not bound to follow the format of the generated test suite and may use the format of their own. Should we track the changes via git?
If the existing case is deleted from the canonical-data.json, what should be done with the existing test functions?

coriolinus · 2018-09-18T11:37:03Z

I think this tool should never delete a test case. It can't easily know if a case not represented in the canonical data was deleted from the canonical data, or was a custom case implemented for the Rust track which has never appeared in the canonical data. For updating existing test cases, I'd favor a very simple--even stupid--strategy: 1. capture the bytes of the existing test case in a buffer: the literal text of the test 2. compare them to the bytes of the generated test case 3. if the generated test case differs from the existing test case, update the name of the generated case with some suffix and then emit them both. Users of this script will then have the implicit option: keep all the cases, and accept some repetition, or manually remove duplicate cases. That's on them. Both of these preferences are consistent with the general philosophy that test suites should be write-only.

…

On Tue, Sep 18, 2018 at 11:50 AM Zapolsky Anton ***@***.***> wrote: Questions regarding the update subcommand: As I understand it, the desirable algorithm is to get the latest canonical-data.json for the exercise and to apply any diffs to the existing test suite: - If a new case/property is added to the canonical-data, add the appropriate functions to the existing test suite - no problems here - If the existing case is updated, add a new function to the test suite and do not touch the existing function. Question: How to track the updates to the cases? We cannot use the existing test suite, as the person who implements the exercise is not bound to follow the format of the generated test suite and may use the format of their own. Should we track the changes via git? - If the existing case is deleted from the canonical-data.json, what should be done with the existing test functions? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#660 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHdeTsAWClySLXJELFCgWW1-fqefRRAvks5ucMHxgaJpZM4WtiY4> .

coriolinus · 2018-09-29T14:59:46Z

bin/build_exercise_crate.sh

+
+	echo "Copying exercise crate from $EXERCISE_CRATE_PATH/target/release/exercise into $BIN_DIR_PATH"
+
+	cp "$EXERCISE_CRATE_PATH/target/release/exercise" "$BIN_DIR_PATH"


This will fail on a Windows machine if the user is running this through cygwin: the binary in that case would be exercise.exe.

…builds

…unction

…e subcommand

ZapAnton · 2018-10-19T14:42:51Z

Some questions regarding usage notes:

update inserts a //NEW line, which I am inclined against keeping. This is somewhat debatable, though.

I have included the line as per your comment:

if the generated test case differs from the existing test case, update
the name of the generated case with some suffix and then emit them both.

The line will change to //UPDATED if the test case was already present in the test suite.
Is this behavior undesirable?

update inserts new process_x_case at the end of the file. I'd prefer it to be inserted at the beginning, before any tests.

By 'before any tests' do you mean the tests generated with the update subcommand, or the already present test suite?
Because if it is the later, I think it is not the best behavior, since we ungroup the changes made to the test suite and scatter them over the whole file, which could be inconvenient for the contributor who applies the changes. And, since there is no standard format for the test suite, it will be hard to guess where to insert the function, which could breaks the existing formatting and is also inconvenient .

when generating process_X_case functions, the current implementation causes the compiler to complain about unused variables. We should, instead of generating comments describing what it might look like, just generate the example directly. I.e. using the abbreviate case:

Since we will be implementing the process_x_function and the current track policy is to always have a non-empty stub file, should we also generate stub template?

If we will be tracking the changes made to the utility via Travis script should some sort of versioning be included?
When generating or updating the test suite, the utility tries to invoke rustfmt on the modified file, which includes the search of the rustfmt using the which command and, if rustfmt is found, applying the formatting. Because of the which command usage, this process is Unix-specifix and will not work on Windows. Would it be enough, if I replace the which command with searching the PATH environment variable?

ZapAnton · 2018-10-19T14:49:46Z

Additional note about the test suite format:

In the update subcommand, I expect the existing test cases to have names like test_description_from_the_canonical_data.
Since there is no standard for formatting and naming the test cases, not all exercises adhere to the above rule.
For example if we try to invoke the update subcommand on the leap exercise we will end up with the whole test suite copied.
Should I left this as it is, and if not should some sort of test naming policy be applied to the track?

…t was modified

ZapAnton · 2018-10-19T15:56:40Z

The Travis script has been added and it seems like it works, but it takes about 5 minutes to complete, which is quite a lot.
Should I change the cargo build --release command with the cargo check command in this script?

coriolinus · 2018-10-19T16:02:31Z

re: inserting //NEW: I see where the confusion came from. I spoke unclearly. My intent was this:
- if the generated name for a particular test is test_description_from_the_canonical_data, and there already exists a test named test_description_from_the_canonical_data, then check if the text of the test is equal to the newly generated test text:
  - if the text of the test is equal to the generated test text, then nothing has changed, so we leave the old test in place and do not write the new test
  - if the text of the test is not equal to the generated test text, then something has changed. We can't expect an automatic tool to know which to keep, so we should keep the existing test, change the name by appending a suffix, and insert the generated test. The format I expect is test_description_from_the_canonical_data_N, where N is the lowest integer > 1 which does not generate a name collision
- if the generated name for a particular test is test_description_from_the_canonical_data, and there already exists a test named description_in_other_words, an automatic tool can't be expected to notice that the test exists already, so simply retain the existing test and also insert the generated test. The tool user or the PR reviewer are the ones who should catch those cases.
given the rules above, we need neither //NEW nor //UPDATED.
re: test file layout. In my opinion, the test file is easiest for a human to read and understand if it contains the following sections in sequence:
- test module doccomment
- imports
- process_X_case implementations
- tests
I understand that maintaining this format will break up the inserted lines into multiple chunks. That's fine with me. There are plenty of diff tools which make the work of finding insertions quite easy.

From the exercise tool's point of view, the rules above require parsing the test file to determine which tests exist, and what bytes they contain, anyway. Given all that information, I think it should not be difficult to insert new process_X_case implementations at the end of the appropriate section.
I do understand that parsing the existing test file is a non-trivial task; implementing these layout and naming rules will not be required to merge this PR. After merging this PR, I'll create a bunch of issues for items which didn't make it into the PR; at that point, I'll probably start work on implementing the rules above myself.
re: generating a stub template. We could think about that for future work, but I think it's beyond the scope of this initial PR. It would be tricky to get right.
re: versioning the script: I don't know exactly what you mean
re: searching the PATH instead of using which: yes, that's a good idea. I'd be happy to see it in this PR, but it could also be an issue after merging this. I don't think that windows support is a big enough issue for this to block merging this PR.
re: replacing cargo build --release with cargo check in travis script: yes, that's a good idea

ZapAnton · 2018-10-19T18:00:13Z

re: versioning the script: I don't know exactly what you mean

What I wanted to say is that from the start the exercise crate had the 0.1.0 version. If we are including the script to check for crate's modifications, should we somehow track and change the crate's version?

coriolinus · 2018-10-19T18:10:53Z

I see. No, I think it's probably best to manually update the crate's version. I'd be very reluctant to give Travis push access to this repo.

ZapAnton · 2018-10-19T18:21:22Z

Then, right before the merge, I will give the crate the 1.0.0 version, to emphasize it's relative completness

…th the 'cargo check' command

coriolinus · 2018-10-19T18:30:43Z

Works for me.

…

On Fri, Oct 19, 2018 at 8:29 PM Zapolsky Anton ***@***.***> wrote: Then, right before the merge, I will give the crate the 1.0.0 version, to emphasize it's relative completness — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#660 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHdeTngkewL5StDj3V_E_t9uCU2ajUFLks5umhgkgaJpZM4WtiY4> .

coriolinus

Thanks, @ZapAnton, for this utility! This may be the biggest PR yet merged to this repo in one go.

There are still quite a few areas which could be improved, but this already more feature-complete than the init_exercise.py script which it replaces.

I confirm that the three critical points I listed above have all been fixed. I've listed some minor changes and fixes below. When the time comes to merge this, I will also go through and add a bunch of issues, itemizing the remaining improvement points that I've found here.

This PR has been open for long enough that I do not intend to wait for other maintainer feedback. I will merge it as soon as you say it's ready, @ZapAnton.

Optionally, you may also delete bin/init_exercise.py as part of this PR.

coriolinus · 2018-10-18T20:04:29Z

exercise/Cargo.toml

+[package]
+name = "exercise"
+version = "0.1.0"
+authors = ["ZapAnton <[email protected]>"]


Please remove this line, as noted in the notes from usage.

Suggested change

authors = ["ZapAnton <[email protected]>"]

coriolinus · 2018-10-19T08:27:24Z

exercise/src/utils.rs

+
+    let exercises_path = Path::new(&track_root).join("exercises");
+
+    for entry in exercises_path


Could we check whether the exercises path exists directly, instead of needing to iterate over the entire path?

coriolinus · 2018-10-19T08:28:07Z

exercise/src/cmd/update.rs

+pub fn update_exercise(exercise_name: &str, use_maplit: bool) {
+    if !utils::exercise_exists(exercise_name) {
+        println!(
+            "Exercise with the name '{}' does not exists. Aborting",


Typo: exists -> exist.

Suggested change

"Exercise with the name '{}' does not exists. Aborting",

"Exercise with the name '{}' does not exist. Aborting.",

coriolinus · 2018-10-19T14:03:25Z

exercise/src/cmd/configure.rs

+        let insert_index = exercises_with_similar_difficulty[insert_index].0;
+
+        let prompt = if insert_index == 0 {
+            format!("{} is the easiest exercise on the track.", exercise_name)


This is only true if *difficulty == 1.

Suggested change

format!("{} is the easiest exercise on the track.", exercise_name)

format!("{} is the easiest exercise of difficulty {}.", exercise_name, *difficulty)

coriolinus · 2018-10-19T14:04:04Z

exercise/src/cmd/configure.rs

+        let prompt = if insert_index == 0 {
+            format!("{} is the easiest exercise on the track.", exercise_name)
+        } else if insert_index == exercises.len() - 1 {
+            format!("{} is the hardest exercise on the track.", exercise_name)


This is only true if difficulty == 10.

Suggested change

format!("{} is the hardest exercise on the track.", exercise_name)

format!("{} is the hardest exercise of difficulty {}.", exercise_name, *difficulty)

coriolinus · 2018-10-19T18:44:19Z

util/exercise/src/cmd/generate.rs

+};
+use utils;
+
+static GITIGNORE_CONTENT: &'static str = "# Generated by Cargo


Strictly speaking this file will not be generated by Cargo

Suggested change

static GITIGNORE_CONTENT: &'static str = "# Generated by Cargo

static GITIGNORE_CONTENT: &'static str = "# Generated by exercism rust track exercise tool

coriolinus · 2018-10-19T19:01:45Z

util/exercise/src/utils.rs

+    false
+}
+
+// Update the version of the specified exersice in the Cargo.toml file according to the passed canonical data


typo

Suggested change

// Update the version of the specified exersice in the Cargo.toml file according to the passed canonical data

// Update the version of the specified exercise in the Cargo.toml file according to the passed canonical data

coriolinus · 2018-10-19T19:05:35Z

util/exercise/src/utils.rs

+};
+use toml::Value as TomlValue;
+
+pub fn get_track_root() -> String {


This might be a good place to use lazy_static! to initialize this value exactly once, and then keep it. By my count this command gets called in eight different places in this code; since the result will never change, we don't need to rerun the subcommand every time.

coriolinus · 2018-10-19T19:13:25Z

util/exercise/src/cmd/generate.rs

+        {} \n\
+        ",
+        exercise_name,
+        "https://github.com/exercism/rust/tree/master/util/exercise",


This could be inlined.

coriolinus · 2018-10-19T19:14:10Z

util/exercise/src/cmd/generate.rs

+        ",
+        exercise_name,
+        "https://github.com/exercism/rust/tree/master/util/exercise",
+        format!("https://raw.githubusercontent.com/exercism/problem-specifications/master/exercises/{}/canonical-data.json", exercise_name),


This could be inlined; it's a little silly to embed a format!() string in the args of another format!() string.

…command with PATH variable parsing

… utility

…_ROOT value

…ependency

ZapAnton · 2018-10-19T22:03:24Z

The removal of the bin/init_exercise.py has triggered the brach conflict. Should I rebase against master?

ZapAnton · 2018-10-19T22:09:33Z

Ignoring the conflict with the Python script, I think I have addressed all the recent points.

@coriolinus if the last batch of commits does not contain any shorcomings and you are fine with moving the remaining usage and formatting notes into separate issues, I think this PR is ready to be merged.

This reverts commit d8d464b. That commit caused a merge conflict.

* extract conceps from leap * Update languages/reference/exercise-concepts/leap.md Co-Authored-By: Peter Goodspeed-Niklaus <[email protected]> Co-authored-by: Peter Goodspeed-Niklaus <[email protected]>

ZapAnton added 21 commits August 31, 2018 14:59

init_exercise: Generated the crate with 'cargo new'

c0da7d5

init_exercise: Added command-line arguments via clap

14520c3

init_exercise: Added configure command

4d28d69

init_exercise: Replaced the dont-update-config flag with the no-confi…

311fe16

…gure flag

init_exercise: Added init_app and process_matches functions

8056b7b

init_exercise: Added cmd module

45449e4

init_exercise: Added process_matches to the generate module

bfadb09

init_exercise: Added exercise template generation

ac3961d

init_exercise: Added test suite and example.rs generation

f6eced6

init_exercise: Added get_canonical_data function

11faf12

init_exercise: Added generate_standard_exercise_template function

9b61818

init_exercise: Renamed generate_standard_exercise_template to generat…

bd002b9

…e_default_meta

init_exercise: Added update_cargo_toml function

907c4b1

init_exercise: Added prepend text to the test suite

fb4ceaa

init_exercise: Added generation of the property functions to the test…

f08f7e2

… suite

init_exercise: Applied suggestions from clippy

a97e032

init_exercise: Added test suite functions generation

bc83644

init_exercise: Added generate_readme function

c1eba7a

init_exercise: Configured std::process::Command calls to wait until t…

13f4778

…he process is finished running

init_exercise: Renamed 'init_exercise' crate to 'exercise' crate

3812526

exercise: Added shell script for building exercise crate

1c25970

coriolinus reviewed Sep 29, 2018

View reviewed changes

ZapAnton added 6 commits October 3, 2018 11:01

exercise: Updated build_exercise_crate.sh script to look for Windows …

e3e39f7

…builds

exercise: Removed redundant echos from build_exercise_crate.sh script

e531e70

exercise: Removed run_configure argument from the generate_exercise f…

831d4cb

…unction

exercise: Added configure_exercise call to the generate subcommand

1400be9

exercise: Added utils module

fe79622

exercise: Implemented the user configuration reading for the configur…

fab6d41

…e subcommand

travis: Added a script to check if the 'exercise' crate compiles if i…

c9e30d2

…t was modified

check-exercise-crate: Replaced the 'cargo build --release' command wi…

7113651

…th the 'cargo check' command

coriolinus approved these changes Oct 19, 2018

View reviewed changes

ZapAnton added 11 commits October 19, 2018 22:25

exercise: Replaced the locating of the 'rustfmt' utility via 'which' …

f23c5e8

…command with PATH variable parsing

exercise: Simplified the exercise_exists function

18131cb

exercise: Fixed typo in the 'exercise does not exist' message

01f3de2

exercise: Specified the exercise difficulty prompt

048d369

exercise: Specified that the new exercise is generated via 'exercise'…

fab2229

… utility

exercise: Fixed comment typo

9850f9b

exercise: Inlined the urls in the generated tests suite template

967bc98

exercise: Replaced the get_track_root function with lazy_static TRACK…

cf3d151

…_ROOT value

README: Updated the 'exercise' crate section to mention the openssl d…

dab2ebf

…ependency

exercise: Updated the crate's version to '1.0.0'

6f73c97

init_exercise.py: Removed the script from the track

d8d464b

coriolinus added 2 commits October 20, 2018 13:12

Revert "init_exercise.py: Removed the script from the track"

e0778ee

This reverts commit d8d464b. That commit caused a merge conflict.

fix readme possessive typo

34ff773

coriolinus merged commit 45599b8 into exercism:master Oct 20, 2018

ZapAnton deleted the riir_init_exercise branch October 20, 2018 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite bin/init_exercise.py in Rust #660

Rewrite bin/init_exercise.py in Rust #660

ZapAnton commented Sep 18, 2018

ZapAnton commented Sep 18, 2018

coriolinus commented Sep 18, 2018 via email

coriolinus Sep 29, 2018

ZapAnton commented Oct 19, 2018

ZapAnton commented Oct 19, 2018

ZapAnton commented Oct 19, 2018

coriolinus commented Oct 19, 2018

ZapAnton commented Oct 19, 2018

coriolinus commented Oct 19, 2018

ZapAnton commented Oct 19, 2018

coriolinus commented Oct 19, 2018 via email

coriolinus left a comment

coriolinus Oct 18, 2018

coriolinus Oct 19, 2018

coriolinus Oct 19, 2018

coriolinus Oct 19, 2018

coriolinus Oct 19, 2018

coriolinus Oct 19, 2018

coriolinus Oct 19, 2018

coriolinus Oct 19, 2018

coriolinus Oct 19, 2018

coriolinus Oct 19, 2018

ZapAnton commented Oct 19, 2018 •

edited

Loading

ZapAnton commented Oct 19, 2018


		echo "Copying exercise crate from $EXERCISE_CRATE_PATH/target/release/exercise into $BIN_DIR_PATH"

		cp "$EXERCISE_CRATE_PATH/target/release/exercise" "$BIN_DIR_PATH"


		let exercises_path = Path::new(&track_root).join("exercises");

		for entry in exercises_path

	"Exercise with the name '{}' does not exists. Aborting",
	"Exercise with the name '{}' does not exist. Aborting.",

	format!("{} is the easiest exercise on the track.", exercise_name)
	format!("{} is the easiest exercise of difficulty {}.", exercise_name, *difficulty)

	format!("{} is the hardest exercise on the track.", exercise_name)
	format!("{} is the hardest exercise of difficulty {}.", exercise_name, *difficulty)

	static GITIGNORE_CONTENT: &'static str = "# Generated by Cargo
	static GITIGNORE_CONTENT: &'static str = "# Generated by exercism rust track exercise tool

	// Update the version of the specified exersice in the Cargo.toml file according to the passed canonical data
	// Update the version of the specified exercise in the Cargo.toml file according to the passed canonical data

Rewrite bin/init_exercise.py in Rust #660

Rewrite bin/init_exercise.py in Rust #660

Conversation

ZapAnton commented Sep 18, 2018

Major Tasks

Minor Tasks

ZapAnton commented Sep 18, 2018

coriolinus commented Sep 18, 2018 via email

Choose a reason for hiding this comment

ZapAnton commented Oct 19, 2018

ZapAnton commented Oct 19, 2018

ZapAnton commented Oct 19, 2018

coriolinus commented Oct 19, 2018

ZapAnton commented Oct 19, 2018

coriolinus commented Oct 19, 2018

ZapAnton commented Oct 19, 2018

coriolinus commented Oct 19, 2018 via email

coriolinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZapAnton commented Oct 19, 2018 • edited Loading

ZapAnton commented Oct 19, 2018

ZapAnton commented Oct 19, 2018 •

edited

Loading