Format using prettier #1917

ErikSchierboom · 2022-01-12T08:32:17Z

As a counterpart to #1916, this PR formats the canonical-data.json files used prettier.

wolf99 · 2022-01-12T08:51:44Z

exercises/all-your-base/canonical-data.json

@@ -153,7 +153,7 @@
        "digits": [0],
        "outputBase": 10
      },
-      "expected": {"error": "input base must be >= 2"}
+      "expected": { "error": "input base must be >= 2" }


Seems like prettier is a lot less opinionated about JSON than jq. That's good for arrays, but bad for cases like this where it would be nice to have fields (error in this case) on their own lines.

Does prettier have any options to force this ?

I don't think so. Prettier has (purposefully) few options: https://prettier.io/docs/en/options.html

SaschaMann

I feel like array formatting should just be left to humans since neither tool seems to do a good job when formatting this kind of human-readable data.

exercises/connect/canonical-data.json

exercises/diamond/canonical-data.json

exercises/dominoes/canonical-data.json

SaschaMann · 2022-01-12T22:39:28Z

Perhaps #1705 should also be considered when choosing a formatter since there was agreement on an order. It would be nice if the tool could also sort the data according to the spec.

ErikSchierboom · 2022-01-13T07:53:08Z

Perhaps #1705 should also be considered when choosing a formatter since there was agreement on an order. It would be nice if the tool could also sort the data according to the spec.

The funny thing is that I started working on that and have a working jq script to re-order fields. That script though, re-formatted a bunch of files in the process, which is why I started looking into the formatting of the files.

I feel like array formatting should just be left to humans since neither tool seems to do a good job when formatting this kind of human-readable data.

Yes, I agree. I think the end conclusion is that we can do a one-time pass for consistent formatting, but with manual tweaks of the results.

ErikSchierboom · 2022-01-13T13:48:35Z

I've manually tweaked the formatting.

SaschaMann

I don't like the error change but that's personal preference; I don't have any real objections against it. I think for everything else, this is a lot better than what either tool produced without the manual tweaks.

exercises/book-store/canonical-data.json

exercises/change/canonical-data.json

exercises/isogram/canonical-data.json

exercises/reverse-string/canonical-data.json

exercises/variable-length-quantity/canonical-data.json

ErikSchierboom · 2022-01-13T14:46:09Z

I don't like the error change but that's personal preference

I don't massively care either way, but I thought people would like this better 🤷 If someone else wants to chip in, I'm happy to change

petertseng · 2022-01-14T23:14:21Z

exercises/bowling/canonical-data.json

+      "description": "all strikes is a perfect game",
+      "property": "score",
+      "input": {
+        "previousRolls": [10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10]


in bowling, is there some rule I can use to understand when the elements of previousRolls are (or should be) on the same line as the [ and ] versus when they are on different lines?

There isn't. I've made them consistent.

petertseng · 2022-01-14T23:22:46Z

exercises/scale-generator/canonical-data.json

@@ -15,7 +15,20 @@
          "input": {
            "tonic": "C"
          },
-          "expected": [ "C", "C#", "D", "D#", "E", "F", "F#", "G", "G#", "A", "A#", "B" ]
+          "expected": [


I can't come up with a rule for why the chromatic scale starting at C should be on separate lines but F should be on one line. They have the same number of notes and the same sum of string lengths. What is the rule?

petertseng · 2022-01-14T23:27:50Z

exercises/word-search/canonical-data.json

-        "wordsToSearchFor": [
-          "clojure"
-        ]
+        "grid": ["jefblpepre", "tclojurerm"],


although putting them on one line is fine because this case doesn't ask to search for a word vertically, I'm kind of thinking that tracks might want to start presenting each on one line, to start introducing the idea that this should be a grid. And if tracks are going to start doing it, I suggest it's a good idea for the JSON to do it as well

Agreed. I've now applied the same multi-line formatting for all array fields. Let me know what that looks like.

petertseng

Do I understand correctly that we cannot add CI checks because we are making case-by-case decisions for arrays? That's too bad, because that means there's a possibility of future changes being badly formatted. Given that, I'm not excited about this PR; I only feel completely ambivalent about it. But if someone really wants this, I support that desire, and therefore I approve.

petertseng · 2022-01-19T18:06:01Z

exercises/grade-school/canonical-data.json

@@ -37,7 +37,11 @@
      "description": "Adding multiple students in the same grade in the roster",
      "property": "add",
      "input": {
-        "students": [["Blair", 2], ["James", 2], ["Paul", 2]]


had I been the one making this PR, I would have kept the existing formatting, but I have no disagreement with the decision made in this PR.

petertseng · 2022-01-19T18:10:16Z

exercises/proverb/canonical-data.json

+        "strings": [
+          "nail",
+          "shoe",
+          "horse",
+          "rider",
+          "message",
+          "battle",
+          "kingdom"
+        ]


I see that unlike the other "strings" in this file, this one qualified to be on separate lines because it has many elements (so putting all on one line would make long). okay.

petertseng · 2022-01-19T18:11:37Z

exercises/reverse-string/canonical-data.json

-  "comments": [
-        "If property based testing tools are available, a good property to test is reversing a string twice: reverse(reverse(string)) == string"
-      ],
+  "comments": ["If property based testing tools are available, a good property to test is reversing a string twice: reverse(reverse(string)) == string"],


showing a place where a long comment was chosen to be the same line as the [ and ]

petertseng · 2022-01-19T18:13:51Z

exercises/isogram/canonical-data.json

+      "comments": [
+        "This test aims to catch buggy implementations that check for duplicate spaces or hyphens."
+      ],


in contrast to a different place where a long comment was chosen to be on the same line as the [ and ], I see that here it was chosen to be on different lines than [ and ]. I don't have an explanation, but I don't need one.

ErikSchierboom · 2022-01-20T09:06:59Z

Do I understand correctly that we cannot add CI checks because we are making case-by-case decisions for arrays? That's too bad, because that means there's a possibility of future changes being badly formatted. Given that, I'm not excited about this PR; I only feel completely ambivalent about it. But if someone really wants this, I support that desire, and therefore I approve.

This PR was mostly a test to check whether the auto formatting used by prettier could be used to auto-format all files and what that would look like. Looking at the review comments, it looked like there were multiple cases where the formatting was not what we wanted. The way I envision this PR going forward was:

Auto format all files using prettier. This will normalize the formatting of all files
Do a manual check to find the files which formatting we feel is wrong. Fix that formatting to what we want

Once we've manually updated the files, we can exclude those files from being auto-formatted. This means that we would be able to have a CI check for the formatting, although there would still be a risk of the excluded files' formatting getting out of sync. Would this approach excite you slightly more?

wolf99 · 2022-01-20T10:53:53Z

exercises/hamming/canonical-data.json

  "comments": [
    "Language implementations vary on the issue of unequal length strands.",
    "A language may elect to simplify this task by only presenting equal",


The leading spacing here looks a bit odd. But I'm on mobile so not sure and it is not worth blocking this PR for.

petertseng · 2022-01-20T19:35:34Z

Well, having some files be checked is better than none, I think. So that is good.

I'm even getting some ideas such as: Even though it appears prettier doesn't allow configuring whether array elements should go on their own, line, perhaps there is something we can write that would do that... then, we can use that tool in conjunction with prettier. We'd have a fixed list of exercises that tool should be run on, and it would run after prettier. In that way, every file will be automatically formatted.

ErikSchierboom · 2022-01-25T07:34:42Z

I'm even getting some ideas such as: Even though it appears prettier doesn't allow configuring whether array elements should go on their own, line, perhaps there is something we can write that would do that... then, we can use that tool in conjunction with prettier. We'd have a fixed list of exercises that tool should be run on, and it would run after prettier. In that way, every file will be automatically formatted.

Ooh, that would be nice. Just on the off chance: would you potentially be interested in developing such a tool? Or do you have ideas on whether we could something like this with plain bash?

petertseng · 2022-01-25T10:34:24Z

Remind me what prettier does? Does it put all the elements on one line?

Now this tool does need to respect indentation, so it may be a little involved for a shell script, though a shell script might be able to do it by counting the number of leading spaces in the line and adding two more.

Now let's see. We're already running Python in CI, in https://github.com/exercism/problem-specifications/blob/main/bin/check-immutability.py, but there's a difference here in that check-immutability.py mainly needs to be run in CI, whereas it may be handy for contributors to be able to run this formatting thing, so a decision should be made with that in mind.

I think it will be easier to get a better sense of what this should look like after formatting starts getting enforced in CI, with exercises excluded. Then, exercises can be un-excluded and this formatting tool can be tested. It's fine to keep talking about it in the meantime though.

One thing I'm thinking about is whether it'll have any problems with strings that contain either [ or , in them, which may start posing more difficulty for a shell script, but I will need to think about that more.

ErikSchierboom · 2022-01-25T10:36:07Z

Remind me what prettier does? Does it put all the elements on one line?

I think it depends on the number of characters the array would take if presented on a single line.

ErikSchierboom · 2022-01-25T11:05:18Z

I've added a couple of commits:

canonical-data.schema.json

petertseng

in the ideal world, the commit entitled "Allow testing of formatting" would have been made easier to review by putting the lines that add the integrity line and remove the URL portion after the # into their own commit, but the problem was not so serious that I was forced to not approve.

Please consider the experience for a contributor who wants to make an addition to the JSON files but does not format it the same way prettier would and therefore CI fails. Is the output informative enough to tell them how to remedy their problem? I will trust your judgment on what you decide is sufficient for you to say we have considered their experience.

ErikSchierboom · 2022-02-01T11:44:53Z

I've added a yarn command to format the JSON and updated the contributing documentation.

I'll add a GHA workflow to allow maintainers to format JSON using a comment on a PR.

ErikSchierboom · 2022-02-01T11:51:38Z

With 3 approvals, I'm gonna merge this and do a GHA workflow formatting follow-up PR later. Thanks everyone!

iHiD

Approving on principle.

ErikSchierboom · 2022-02-01T12:04:48Z

Thanks everybody!

* Format using prettier (exercism#1917) Format using prettier * updated description of anagrams exercise (exercism#1928) * updated description of anagrams * changed anagram description to be one-sentence-per-line * updated description of anagrams to use sets * Update Licence Give a look at the discussion in BR exercism#1930 * rational-numbers: test to reduce abs value (exercism#1938) * Change saddle point references to row, column (exercism#1948) * word-search: Add test case * Update exercises/word-search/canonical-data.json Agreed. Co-authored-by: Erik Schierboom <[email protected]> * meetup: improve descriptions by saying why each case is tested (exercism#1919) descriptions show whether a date is the first, last, or an arbitrary middle date of the week. This helps understand why certain cases are selected. Closes exercism#974 * word-search: Add cases checking for concatenation and wrapping The author of this commit thinks that concatenation is highly unlikely, but the wrapping might be useful to check in languages that allow negative indices. * `flatten-array` Add additional test cases (exercism#1953) * Add additional test cases to flatten-array * Update exercises/flatten-array/canonical-data.json Co-authored-by: Peter Tseng <[email protected]> Co-authored-by: BethanyG <[email protected]> Co-authored-by: Peter Tseng <[email protected]> * Fix bowling game copy (exercism#1955) Fixes exercism#1954 * Add action to format code (exercism#1941) * build(deps): bump DavidAnson/markdownlint-cli2-action (exercism#1952) Bumps [DavidAnson/markdownlint-cli2-action](https://github.com/DavidAnson/markdownlint-cli2-action) from 5.0.0 to 5.1.0. - [Release notes](https://github.com/DavidAnson/markdownlint-cli2-action/releases) - [Commits](DavidAnson/markdownlint-cli2-action@b3c3b40...744f913) --- updated-dependencies: - dependency-name: DavidAnson/markdownlint-cli2-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Reduced rational nr. should be in standard form. (exercism#1958) * Reduced rational should be in standard form. The current instructors fail to mention that a reduced rational number should always be rendered in standard form (without any negative value at the denominator). * remove superflous blank lines; fix wording * scale-generator: use flat and sharp symbols (exercism#1942) * Update configlet part in README (exercism#1949) Co-authored-by: ee7 <[email protected]> * phone number: only one problem per test input (exercism#1959) * [Phone Number] Only one problem per test input Because the area code is not allowed to start with 0 or 1, inputs designed to elicit other errors should not use area codes that start with either of those digits. * Respect immutability * Correct field name: s/comment/comments/ * Comments should contain a list. * Allow prettier to improve comments * book-store: reorder keys * darts: reorder keys * grade-school: reorder keys * hamming: reorder keys * high-scores: reorder keys * largest-series-product: reorder keys * list-ops: reorder keys * luhn: reorder keys * triangle: reorder keys * scale-generator: reorder keys * saddle-points: reorder keys * diffie-hellman: reorder keys * collatz-conjecture: reorder keys * anagram: reorder keys * accumulate: reorder keys * Add CI script to check correct order of keys Co-authored-by: Bart Massey <[email protected]> Co-authored-by: y8l <[email protected]> Co-authored-by: Ivan Ivanov <[email protected]> Co-authored-by: Damian C. Rossney <[email protected]> Co-authored-by: mariohuq <[email protected]> Co-authored-by: mariohuq <[email protected]> Co-authored-by: Peter Tseng <[email protected]> Co-authored-by: Peter Tseng <[email protected]> Co-authored-by: AH WEI <[email protected]> Co-authored-by: BethanyG <[email protected]> Co-authored-by: Cedd Burge <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Davide Alberto Molin <[email protected]> Co-authored-by: wolf99 <[email protected]> Co-authored-by: June <[email protected]> Co-authored-by: ee7 <[email protected]> Co-authored-by: Leah Hanson <[email protected]>

ErikSchierboom mentioned this pull request Jan 12, 2022

Format canonical data using jq #1916

Closed

wolf99 reviewed Jan 12, 2022

View reviewed changes

SaschaMann reviewed Jan 12, 2022

View reviewed changes

exercises/connect/canonical-data.json Outdated Show resolved Hide resolved

exercises/diamond/canonical-data.json Outdated Show resolved Hide resolved

exercises/dominoes/canonical-data.json Outdated Show resolved Hide resolved

ErikSchierboom force-pushed the format-prettier branch 2 times, most recently from 7fbc4ad to a08f3c8 Compare January 13, 2022 13:47

ErikSchierboom requested review from SaschaMann and wolf99 January 13, 2022 13:48

SaschaMann reviewed Jan 13, 2022

View reviewed changes

ErikSchierboom requested a review from SaschaMann January 14, 2022 09:24

ErikSchierboom marked this pull request as ready for review January 14, 2022 09:24

ErikSchierboom requested a review from SleeplessByte January 14, 2022 13:33

petertseng reviewed Jan 14, 2022

View reviewed changes

petertseng approved these changes Jan 19, 2022

View reviewed changes

wolf99 approved these changes Jan 20, 2022

View reviewed changes

SaschaMann approved these changes Jan 25, 2022

View reviewed changes

ErikSchierboom requested a review from a team as a code owner January 25, 2022 11:03

ErikSchierboom requested review from petertseng and wolf99 January 25, 2022 11:05

SaschaMann reviewed Jan 25, 2022

View reviewed changes

canonical-data.schema.json Show resolved Hide resolved

petertseng approved these changes Jan 25, 2022

View reviewed changes

ErikSchierboom added 17 commits February 1, 2022 09:02

sgf-parsing: format

9083cd1

simple-cipher: format

4edc136

space-age: format

d9856f8

spiral-matirx: format

0d72ce4

tournament: format

5f7616e

variable-length-quantity: format

3e4e9c9

word-search: format

131d573

wordy: format

510e72e

Apply review comments

387594c

Fix error expected value indentation

7c78d2c

bowling: use consistent array formatting

83c1467

scale-generator: use consistent array formatting

9725d3f

word-search: use consistent array formatting

2b786c0

Ignore manually formatted files

dd229f5

Format non-exercise JSON files

6ee1de2

Allow testing of formatting

467b34b

Add CI to formatting

2be235c

ErikSchierboom force-pushed the format-prettier branch from 0b470a1 to 2be235c Compare February 1, 2022 08:02

ErikSchierboom added 2 commits February 1, 2022 12:42

Add command to format JSON

ddfc0f9

Add documentation on how to format JSON

fbd495f

ErikSchierboom mentioned this pull request Feb 1, 2022

Add action to format code #1941

Merged

iHiD approved these changes Feb 1, 2022

View reviewed changes

ErikSchierboom merged commit d137db1 into main Feb 1, 2022

ErikSchierboom deleted the format-prettier branch February 1, 2022 12:04

petertseng mentioned this pull request Feb 1, 2022

sgf-parsing: Add individual tests of escaping/whitespace behaviour #1889

Merged

SaschaMann mentioned this pull request Feb 17, 2022

allow prettier to format more files #1966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format using prettier #1917

Format using prettier #1917

ErikSchierboom commented Jan 12, 2022 •

edited

Loading

wolf99 Jan 12, 2022

ErikSchierboom Jan 12, 2022

SaschaMann left a comment

SaschaMann commented Jan 12, 2022

ErikSchierboom commented Jan 13, 2022

ErikSchierboom commented Jan 13, 2022

SaschaMann left a comment

ErikSchierboom commented Jan 13, 2022

petertseng Jan 14, 2022

ErikSchierboom Jan 19, 2022

petertseng Jan 14, 2022

ErikSchierboom Jan 19, 2022

petertseng Jan 14, 2022

ErikSchierboom Jan 19, 2022

petertseng left a comment

petertseng Jan 19, 2022

petertseng Jan 19, 2022

petertseng Jan 19, 2022

petertseng Jan 19, 2022

ErikSchierboom commented Jan 20, 2022

wolf99 Jan 20, 2022

petertseng commented Jan 20, 2022

ErikSchierboom commented Jan 25, 2022

petertseng commented Jan 25, 2022

ErikSchierboom commented Jan 25, 2022

ErikSchierboom commented Jan 25, 2022 •

edited

Loading

petertseng left a comment

ErikSchierboom commented Feb 1, 2022 •

edited

Loading

ErikSchierboom commented Feb 1, 2022

iHiD left a comment

ErikSchierboom commented Feb 1, 2022

Format using prettier #1917

Format using prettier #1917

Conversation

ErikSchierboom commented Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaschaMann left a comment

Choose a reason for hiding this comment

SaschaMann commented Jan 12, 2022

ErikSchierboom commented Jan 13, 2022

ErikSchierboom commented Jan 13, 2022

SaschaMann left a comment

Choose a reason for hiding this comment

ErikSchierboom commented Jan 13, 2022

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

petertseng 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

ErikSchierboom commented Jan 20, 2022

Choose a reason for hiding this comment

petertseng commented Jan 20, 2022

ErikSchierboom commented Jan 25, 2022

petertseng commented Jan 25, 2022

ErikSchierboom commented Jan 25, 2022

ErikSchierboom commented Jan 25, 2022 • edited Loading

petertseng left a comment

Choose a reason for hiding this comment

ErikSchierboom commented Feb 1, 2022 • edited Loading

ErikSchierboom commented Feb 1, 2022

iHiD left a comment

Choose a reason for hiding this comment

ErikSchierboom commented Feb 1, 2022

ErikSchierboom commented Jan 12, 2022 •

edited

Loading

ErikSchierboom commented Jan 25, 2022 •

edited

Loading

ErikSchierboom commented Feb 1, 2022 •

edited

Loading