Skip to content

add format-array (formats JSON arrays) #1968

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 31, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/action-format.yml
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ jobs:
run: yarn install --pure-lockfile

- name: Format JSON files
run: yarn format-json
run: yarn format-json && ruby bin/format-array.rb

- name: 'Commit formatted code'
run: |
Expand Down
8 changes: 7 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,10 @@ jobs:
run: yarn install

- name: Verify that json files are formatted correctly
run: yarn test-json-formatting
run: |
yarn format-json && ruby bin/format-array.rb
if ! git diff --quiet --exit-code; then
echo "please format the files with prettier and bin/format-array.rb, or apply the following changes:"
git diff
exit 1
fi
18 changes: 0 additions & 18 deletions .prettierignore

This file was deleted.

5 changes: 3 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -304,12 +304,12 @@ We are maintaining this section, since many open issues link to it.

## Formatting

This repository uses [prettier][prettier] to automatically format its JSON files.
This repository uses [prettier][prettier] and a [custom array formatter][format-array] to automatically format its JSON files.
If you've added or modified a JSON file, you can format it using:

```shell
yarn install
yarn format-json
yarn format-json && ruby bin/format-array.rb
```

Note: if you use VS Code as your editor, you can install the [prettier plugin][prettier-vs-code] to automatically handle formatting for you.
Expand All @@ -336,4 +336,5 @@ Note: if you use VS Code as your editor, you can install the [prettier plugin][p
[improve-exercise-metadata]: https://github.com/exercism/legacy-docs/blob/main/you-can-help/improve-exercise-metadata.md
[legacy-docs]: https://github.com/exercism/legacy-docs
[prettier]: https://prettier.io/
[format-array]: https://github.com/exercism/problem-specifications/blob/main/bin/format-array.rb
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notice! this link does not work yet for the obvious reason that format-array doesn't exist on main yet! I am not sure if I have a better solution to offer for review, other than "you will just have to check this link very carefully"

Or, this change could be removed from this PR and only submitted in a separate PR after format-array.rb has been merged, so that all can verify that the link is correct, but I was not convinced that was a better solution.

[prettier-vs-code]: https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode
178 changes: 178 additions & 0 deletions bin/format-array.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
require 'json'

# format-array expresses (and enacts) our per-file preferences for array formatting,
# in cases where the preference differs from prettier.
#
# It has only one mode of operation:
# Run it without any arguments to format the files.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may suggest that I could add a --check flag to have it exit 1/0 depending on whether files need to be formatted, so that it can be incorporated into CI

Actually, my plan to incorporate it into CI was just to run it and check git diff. The advantage of that is that we could even show the git diff output in the CI, which will give contributors an idea of what changes they need to make. That seems to me to be more useful than just saying "you need to make some changes".

Copy link
Contributor

@SaschaMann SaschaMann Feb 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've done the same in the past in the v3-repo, you can probably copy parts of the format workflows there to set it up: https://github.com/exercism/v3/blob/main/.github/workflows/format-code.yml


# format-array was written because we want to keep files consistently formatted,
# while also optimising for human readability.
#
# As part of optimising for human readability,
# some arrays should be formatted such that each element is on its own line,
# whereas some other arrays should be formatted such that the entire array is on one line.
#
# We could not find an existing tool that allows us to specify array formatting how we wanted.

# The configuration of array formatting preferences.
#
# format can be the following choices:
#
# * single_line (the entire array and all of its elements should be on a single line)
# * multi_line (each element of the array is on its own line)
# * multi_line_unless_single (like multi_line, except arrays with one element remain on a single line)
# * multi_line_deep (multi_line, even applied to arrays within that array)
# * padded (14 elements per line padded to 3 characters per element)
# (TODO: padding amount could be configurable, but we haven't needed it)
formats = {
'book-store' => {
'basket' => :single_line,
},
'bowling' => {
'previousRolls' => :single_line,
},
'change' => {
'expected' => :single_line,
},
'connect' => {
'board' => :multi_line,
},
'diamond' => {
'expected' => :multi_line,
},
'dominoes' => {
'dominoes' => :single_line,
},
'flatten-array' => {
'array' => :multi_line_deep,
'expected' => :multi_line,
},
'forth' => {
'instructions' => :multi_line_unless_single,
},
'go-counting' => {
'board' => :multi_line,
},
'minesweeper' => {
'minefield' => :multi_line_unless_single,
'expected' => :multi_line_unless_single,
},
'ocr-numbers' => {
'rows' => :multi_line,
},
'rectangles' => {
'strings' => :multi_line_unless_single,
},
'saddle-points' => {
'matrix' => :multi_line,
},
'scale-generator' => {
'expected' => :single_line,
},
'sieve' => {
'expected' => :padded,
},
'transpose' => {
'lines' => :multi_line,
'expected' => :multi_line,
},
'variable-length-quantity' => {
'integers' => :single_line,
'expected' => :single_line,
},
'word-search' => {
'grid' => :multi_line,
'wordsToSearchFor' => :multi_line,
},
}.each_value(&:freeze).freeze

def single_line_arrays(contents, key)
# matches things of the form "key": [1, 2, 3]
# because this is not a multi-line regex,
# . does NOT match newlines.
contents.scan(/^( +)"#{key}": (\[.*\],?$)/)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically speaking, this (using a regex) could also match the text in a comment, but that is both highly unlikely and pretty noticeable in a PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fascinating case to consider. Presumably it would be something like

{
  "comment": [
    "consider the following input",
    "foo\": [1, 2, 3]"
  ]
}

That doesn't particularly look good in JSON, and for this to match you have to pass a key that includes a \... it's indeed pretty noticeable on both ends.

end

def multi_line_arrays(contents, key)
# matches things of the form
# "key": [
# 1,
# 2,
# 3
# ]
# because this IS a multi-line regex (note the /m),
# . DOES match newlines.
#
# To find which closing bracket matches the opening bracket,
# we find the *first* closing bracket that is both:
# - the only thing on its line (except for maybe a comma afterward)
# - at the same indentation level as the key
#
# to find the first of these, make the match non-greedy (*? instead of *)
contents.scan(/^( +)"#{key}": (\[$.*?^\1\],?$)/m)
end

formats.each { |exercise, exercise_format|
filename = "#{__dir__}/../exercises/#{exercise}/canonical-data.json"
contents = File.read(filename)
exercise_format.each { |key, format_type|
replace = ->(old, new) {
# Include the key in both the search and the replacement,
# to avoid accidentally replacing something we didn't mean to.
# This has been observed to be important for transpose,
# where ["A1"] is both an input and an output.
contents.sub!(%Q("#{key}": #{old}), %Q("#{key}": #{new}))
}

case format_type
when :single_line
multi_line_arrays(contents, key).each { |indent, arr|
arr_lines = arr.lines
raise "impossible, array doesn't start with [ but instead #{arr_lines[0]}" if arr_lines[0] != "[\n"
raise "impossible, array doesn't end with ] but instead #{arr_lines[-1]}" unless arr_lines[-1].match?(/^ *\],?$/)
replace[arr, "[#{arr_lines[1...-1].map(&:strip).join(' ')}]#{',' if arr[-1] == ','}"]
}
when :multi_line, :multi_line_unless_single
single_line_arrays(contents, key).each { |indent, arr|
elements = JSON.parse(arr.delete_suffix(','))
next if elements.empty?
next if elements.size == 1 && format_type == :multi_line_unless_single
indented_elements = elements.map { |el|
js = JSON.generate(el)
# JSON.generate will output [1,2,3] but we want [1, 2, 3]
"#{indent} #{el.is_a?(Array) ? js.gsub(',', ', ') : js}"
}
# all lines but the last need a trailing comma
replace[arr, "[\n#{indented_elements.join(",\n")}\n#{indent}]#{',' if arr[-1] == ','}"]
}
when :multi_line_deep
single_line_arrays(contents, key).each { |indent, arr|
# pretty_generate will render an empty array as:
# [
#
# ]
# whereas we just want []
pretty_lines = JSON.pretty_generate(JSON.parse(arr)).sub(/\[\s+\]/, '[]').lines
if pretty_lines == ['[]']
replace[arr, '[]']
else
raise "impossible, array doesn't start with [ but instead #{pretty_lines[0]}" if pretty_lines[0] != "[\n"
raise "impossible, array doesn't end with ] but instead #{pretty_lines[-1]}" if pretty_lines[-1] != "]"
replace[arr, "[\n" + pretty_lines[1...-1].map { |l| "#{indent}#{l}" }.join + "#{indent}]#{',' if arr[-1] == ','}"]
end
}
when :padded
multi_line_arrays(contents, key).each { |indent, arr|
elements = JSON.parse(arr)
padded_elements = elements.map { |el| '%3d' % el }
new_rows = padded_elements.each_slice(14).map { |row| indent + ' ' + row.join(', ') }
# all lines but the last need a trailing comma
replace[arr, "[\n#{new_rows.join(",\n")}\n#{indent}]"]
}
else
raise "unknown #{exercise} #{key} #{format_type}"
end
}
File.write(filename, contents)
}
4 changes: 1 addition & 3 deletions exercises/bowling/canonical-data.json
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,7 @@
"description": "last two strikes followed by only last bonus with non strike points",
"property": "score",
"input": {
"previousRolls": [
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 10, 0, 1
]
"previousRolls": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10, 10, 0, 1]
},
"expected": 31
},
Expand Down
4 changes: 3 additions & 1 deletion exercises/transpose/canonical-data.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@
"description": "single line",
"property": "transpose",
"input": {
"lines": ["Single line."]
"lines": [
"Single line."
]
},
"expected": [
"S",
Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
"scripts": {
"test": "ajv -s canonical-data.schema.json -d \"exercises/*/canonical-data.json\"",
"test-one": "ajv -s canonical-data.schema.json -d",
"test-json-formatting": "prettier --check **/*.json",
"format-json": "prettier --write **/*.json"
},
"resolutions": {
Expand Down