Conversation
|
|
||
| Dir.glob('exercises/*/canonical-data.json').each do |path| | ||
| json = JSON.parse(File.read(path)) | ||
| invalid_tests = find_tests(json).select {|test| (CORRECT_ORDER & test.keys) != test.keys} |
bin/check_key_order.rb
Outdated
|
|
||
| exit_code = 0 | ||
|
|
||
| Dir.glob('exercises/*/canonical-data.json').each do |path| |
There was a problem hiding this comment.
You could parallize this :P
SaschaMann
left a comment
SaschaMann
left a comment
There was a problem hiding this comment.
Can't read the Ruby script but otherwise this looks good
wolf99
left a comment
wolf99
left a comment
There was a problem hiding this comment.
I don't know about the Ruby, but the resulting JSON looks good.
bin/check_key_order.rb
Outdated
|
|
||
| puts "#{path} has tests with the wrong key order:" | ||
| invalid_tests.each do |test| | ||
| puts "- Test: #{test['uuid']}" |
There was a problem hiding this comment.
Perhaps this should include the description similarly to the mutation-check so that it's easier to find without CTRL F
ErikSchierboom
force-pushed
the
reorder-keys
branch
from
43d902a to
03dc630
Compare
|
@SaschaMann I've added the description to the output. How do we want to merge this? As on squashed commit, two commits or many individual commits? My preference would be for the second option, but I'm fine with whatever. |
|
I like it if the resulting changes are bundled with the script, that way it makes it easier to understand what actually changed if you can't read workflows or Ruby files. 1st and 2nd option both seem fine though, the 3rd seems a bit too noisy. |
|
I would just squash it. |
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
No particular preference between two commits vs one.
bin/check_key_order.rb
Outdated
|
|
||
| exit_code = 0 | ||
|
|
||
| Dir.glob('exercises/*/canonical-data.json').each do |path| |
There was a problem hiding this comment.
this has made it so that this can only be run from the root of the problem-specifications repo. If you want to instead make it runnable from anywhere, you should express the path relative from bin.
| Dir.glob('exercises/*/canonical-data.json').each do |path| | |
| Dir.glob("#{__dir__}/../exercises/*/canonical-data.json").each do |path| |
But, I do not actually expect wanting to run it from anywhere else to be something that is often desired, so I'm Approving (in the GitHub sense of the word) anyway regardless of whether this change is made.
There was a problem hiding this comment.
I've applied the change, with a minor change to allow the printed path to be slightly clearer
ErikSchierboom
force-pushed
the
reorder-keys
branch
from
03dc630 to
caff42c
Compare
|
Thanks everyone! |
6 participants
This PR makes the order of the keys in the
canonical-data.jsonfiles consistent and adds a CI check to verify its order.This is just a minor improvement, so if people are not convinced this is useful, I'm happy to not go through with this.
See https://github.com/ErikSchierboom/problem-specifications/runs/5158116886?check_suite_focus=true for an example of the output.
Closes #1705