Impliment new template based generator #1730
Conversation
|
This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested. If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos. For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping |
| @@ -0,0 +1,49 @@ | |||
| require 'toml-rb' | |||
There was a problem hiding this comment.
What problems or solutions are we avoiding/gaining by using TOML as opposed to YAML (or even JSON)? This is the point at which TOML as a tool and dependency is being introduced, and its associated maintenance cost, so we should evaluate that. We should have a net positive effect by bringing this in.
There was a problem hiding this comment.
The test.toml files are written in toml, using another file format isn't really an option since rewriting it would require parsing it.
I wrote a manual parser for the Crystal generator (but it is likely not as good of an implementation) because I couldn't find any good shards (library). This library should be fine, the toml specification isn't changing, but we could have our own solution.
There was a problem hiding this comment.
The goal is more or less with the toml files is that, if someone adds extra test cases or reimplements test cases for a certain exercise will that not affect the generation process until you sync the exercise. Meaning the ci won't fail in the majority of cases if not all exercises are synced.
bin/generate
Outdated
| f = Tempfile.create | ||
| Generator.new(exercise).generate(f.path) | ||
| generated_code = f.read | ||
| raise RuntimeError.new("The result generated for: #{exercise}, doesnt match the current file") if current_code != generated_code |
There was a problem hiding this comment.
| raise RuntimeError.new("The result generated for: #{exercise}, doesnt match the current file") if current_code != generated_code | |
| raise RuntimeError.new("The result generated for: #{exercise}, doesn't match the current file") unless current_code == generated_code |
Typographical error (missing apostrophe) and preference of "positive conditional statement".
Prefer using unless positive_conditional_statement rather than if negative, this can keep all (or at least most) of our conditional statements positive.
We should also consider if RuntimeError is the best error here. It could be VerificationError instead, which would be more specific.
We might also fail instead of raise for the communication that this is purposefully failed here, given the right conditions.
| Since the generator grabs all the data from the canonical data, so does this enable new tests that won't automatically be merged in. | ||
| Instead so does new tests have to be added to the toml file before they show up in the test file. | ||
|
|
||
| If there is a test that isn't needed or something that doesn't fit Ruby you can remove it from the toml file. |
There was a problem hiding this comment.
| If there is a test that isn't needed or something that doesn't fit Ruby you can remove it from the toml file. | |
| If there is a test that isn't needed or something that doesn't fit Ruby you can remove it from the configuration file. |
In case we move from TOML to another type of configuration file, this reference will not need to change in the documentation.
…g as the normal creation. Other changes based on feedback
| end | ||
| rescue VerificationError => e | ||
| $stderr.puts e.message % {exercise:} |
There was a problem hiding this comment.
Be aware that this syntax is relatively new, and so we should ensure that the minimum Ruby version is set to allow for this "valueless hash" syntax.
There was a problem hiding this comment.
I mean it is tooling and we have set the ruby version to 3.3 so I think we dont have to have backwards compatibility.
There was a problem hiding this comment.
It means that it is backward compatible to 3.3.0, up to 3.3.6, currently. Locally I am running 3.3.6, and so the backward compatibility is there.
No description provided.