add format-array (formats JSON arrays) #1968

petertseng · 2022-02-19T05:47:32Z

format-array expresses our per-file preferences for array formatting, in
cases where the preference differs from prettier.

To see that this is true, the following procedure can
be used:

Delete or truncate .pretterignore and run prettier on JSON files.
prettier will have reformatted them.
run format-array. It will restore the files to their original state.

This is ready for review.

The below are historical blockers that had to be solved before it is ready for review, but they are now solved:

allow prettier to format more files #1966 and auto-format portions of some JSON files #1967 need to be merged first (notice that this PR includes commits from both of those PRs). Reviews of this PR should be limited solely to 0196b9e and c384a84. Reviews of other commits should go in their respective PRs. done
~~allergies: format #1978 needs to be merged first~~ done
~~It should be incorporated into CI before being merged (and that will need review when the time comes as well)~~ done

suggested items for review:

comment on whether any portions of the code could be made clearer.
comment on where the per-file preferences should be. I see two main options:
- in a single file (currently, just directly in format-array). Advantage: Most easily allows us to see in one place all the exceptions we're making to prettier and where they are. analogous to current .prettierignore where all the exceptions are in that one file.
- somewhere associated with each exercise (for example, the preferences for book-store could be in exercises/book-store/canonical-data.json or some other file in exercises/book-store). Advantage: If I'm just interested in editing a single exercise's JSON file, I only need to look in that JSON file to find the rules, rather than knowing that I need to look in bin/format-array.rb as well.
a reminder that the reason for the existence of format-array was because we could not find a tool that allowed us to specify array formatting, so we had to do it ourselves. If you do know of such a tool, you should let us know so that we can explore using it instead of format-array. Items to consider:
- https://www.npmjs.com/package/prettier-plugin-multiline-arrays - a recent creation (2022-02-07), which is why we had not heard of it before making this PR. However we would need to conditionally include it based on which file was being formatted, and we also don't really have equivalents for padded, and it can't correctly format dominoes, triangle, or saddle-points, where the multiline-ness should descend only one level deep into an array of arrays.
- the --print-width flag for prettier: I haven't explored this to find out whether we can specify a value for print-width that correctly does what we want. I suspect it would be a little difficult, since our decision is not based on line length or width.

petertseng · 2022-02-19T05:55:14Z

There are a few multi_line that I feel could be changed into multi_line_unless_single, but for now I'm keeping the existing formatting how it is. If you feel some of the multi_line can be changed to multi_line_unless_single, I suggest enacting those changes in a separate PR, where that idea can have its dedicated area for discussion. If your PR gets merged before mine, I will gladly rebase mine and change the rule accordingly. If my PR gets merged first, you will have to rebase so you have format-array, and you wil have to change the rule. I do not plan to enact any multi_line -> multi_line_unless_single changes (or any other rule changes, in fact) in this particular PR.

petertseng · 2022-02-19T06:01:49Z

bin/format-array.rb

+# in cases where the preference differs from prettier.
+#
+# It has only one mode of operation:
+# Run it without any arguments to format the files.


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".

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

ErikSchierboom · 2022-02-22T07:25:14Z

exercises/ocr-numbers/canonical-data.json

+      "expected": {
+        "error": "Number of input columns is not a multiple of three"
+      }


Should this be (similar to the above change):

Suggested change

"expected": {

"error": "Number of input columns is not a multiple of three"

}

"expected": { "error": "Number of input columns is not a multiple of three" }

that would not be in line with prettier. format-array has no intention of formatting objects or other non-array items

ErikSchierboom · 2022-02-22T07:26:10Z

exercises/pascals-triangle/canonical-data.json

@@ -66,7 +66,14 @@
      "input": {
        "count": 6
      },
-      "expected": [[1], [1, 1], [1, 2, 1], [1, 3, 3, 1], [1, 4, 6, 4, 1], [1, 5, 10, 10, 5, 1]]
+      "expected": [


Is it possible to also have the other expected values formatted with one element per line? That way one really gets a visual indication of the triangle shape

with format-array, very possible! I will add it to the list of things to be done after this PR. It will be as simple as having expected be multi_line.

ErikSchierboom · 2022-02-22T07:28:28Z

bin/format-array.rb

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


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.

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.

petertseng · 2022-02-25T02:37:25Z

PR description has been edited to add:

Reviews of this PR should be limited solely to 0196b9e and c384a84. Reviews of other commits should go in their respective PRs.

ErikSchierboom

Approving based on the two mentioned commits.

petertseng · 2022-03-04T12:48:21Z

Now that the other two PRs are merged (I have rebased it on main now that they are, so this PR now only contains the relevant commits), we could go about this two possible ways:

I could take this PR out of drafts right now, and if it gets approved, it can be merged, without waiting for it to be incorporated into CI. The reason to do this would be so reviewers have to review fewer changes at one time.
Or I could leave it as a draft until such time as I do incorporate it into CI. The reason to do this would be that it's less churn if it so happens that I find out only while trying to incorporate it into CI that I need to make a change, and so that reviewers can see the whole picture of how this fits in when reviewing (be able to see how the trees make up the forest, so to speak)

I prefer the latter so I will be doing that unless I hear otherwise.

petertseng · 2022-03-04T13:05:40Z

~~amendment: #1978 will also need to be merged before I would take this PR out of drafts, but I will look toward incorporating this into CI in the coming days.~~ done

petertseng · 2022-03-05T02:55:58Z

I now think this is ready for review. See https://github.com/exercism/problem-specifications/runs/5430593239?check_suite_focus=true , which is the CI run for #1981, to see what happens now if a JSON file's formatting does not match prettier + format-array.

petertseng · 2022-03-05T03:03:44Z

CONTRIBUTING.md

@@ -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


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.

petertseng · 2022-03-05T03:07:18Z

notice! I have just found out that format-array.rb uses hyphens to separate words in its filename, whereas other files in https://github.com/exercism/problem-specifications/tree/main/bin are inconsistent in whether they use hyphens or underscores, but crucially the other Ruby script check_key_order.rb uses underscores. If this is a problem, just say the word and I will change format-array.rb to format_array.rb.

petertseng · 2022-03-05T03:28:56Z

Recall that I also put out a call for anyone to put forth alternative ways of formatting arrays. I have added discussion of two potential options to the initial PR comment (https://www.npmjs.com/package/prettier-plugin-multiline-arrays, and the --print-width flag), but I'm not currently convinced that they are suitable for our needs.

ErikSchierboom

Looks good!

ErikSchierboom · 2022-03-29T10:00:07Z

This has been lying dormant for a while. @exercism/reviewers could you maybe review this PR?

petertseng · 2022-03-29T18:50:36Z

All right! So, I have a weak preference for not squashing these commits. Nobody else who reviewed expressed an opinion either way, so that could mean nobody has an opinion or just that it wasn't on the forefront of their minds at the time, and that's fine. I know that I could ask Erik to merge it without squashing. So @ErikSchierboom I'm going to tentatively ask for that. If it seems better to just squash just say the word and that's fine with me.

ErikSchierboom · 2022-03-30T06:09:02Z

Ah, it turns out I don't have enough permissions to do a merge commit. @iHiD could you merge this without squashing?

iHiD · 2022-03-30T10:07:19Z

I don't have permission for that either :) Can I rebase+merge?

ErikSchierboom · 2022-03-30T10:19:37Z

I think that a rebase merge loses the pull request context? I'm not entirely sure.

SaschaMann · 2022-03-30T10:21:52Z

It's still inked in the UI but not the commit message(s), I think. But I'd just squash it tbh

ErikSchierboom · 2022-03-30T10:23:39Z

@petertseng ?

ee7 · 2022-03-30T12:30:47Z

I can't claim to know @petertseng's current thoughts, but I remember them writing a nice post on this topic years ago.

To expand a little, I'd agree that the best practice is a merge commit in this situation. Ideally after rebasing the PR branch on the most recent main commit, so that:

The history is as simple as possible (git log --graph has minimal noise).
CI tests the merge. Despite the fact that "checks have passed" on this PR, CI hasn't actually checked the result of pressing the big green button - it's only checked the result of merging with a past main commit. This problem isn't limited to this PR - it exists whenever the state of main at merge-time is different from the state of main when CI ran on the PR. See e.g. bors for a fuller explanation. An important takeaway: "no merge conflicts" does not guarantee that a merge is OK.

I think that using the merge commit is the best of all worlds here because:

The commits stay granular, easy to understand, and easy to reference later - with specific commit messages (lacking with "squash and merge").
The git log can contain the PR number and context (lacking with "rebase and merge", unless the commits are written in advance to contain that information).
The commits are applied as a whole (lacking in "rebase and merge", where in general the main branch can then contain some intermediate broken state that e.g. affects the ability to git bisect). And again in general, it also means that if the merge unexpectedly breaks prod (because of suboptimal test coverage or merge practices), you can immediately revert the entire merge commit and ask questions later. That's more painful with "rebase and merge", where again, reverting one commit might produce a broken intermediate state, and so you revert the range of commits (and the fact that you probably want a squashed commit or a merge commit when reverting shows that "rebase and merge" was suboptimal in the first place).

But please consider this post entirely non-blocking. There's a tradeoff between allowing the "best" merge method for every situation, and avoiding a "bad" merge method for the average situation.

bowling has `previousRolls` on a single line. transpose has each element of `lines` on its own line. pull request: exercism#1968

format-array expresses our per-file preferences for array formatting, in cases where the preference differs from prettier. To see that this is true, the following procedure can be used: 1. Delete or truncate .pretterignore and run prettier on JSON files. prettier will have reformatted them. 2. run format-array. It will restore the files to their original state. pull request: exercism#1968

pull request: exercism#1968

with format-array running, we are now ready to allow prettier to format all. pull request: exercism#1968

pull request: exercism#1968

petertseng · 2022-03-30T17:15:47Z

Thanks for linking to my previous post! I think it generally still accurately describes my views. I can also help the situation a little - I have now included a link to the pull request in the commit messages. That solves my complaint with rebase+merge (loses link to pull request), and I would say it'd be okay to rebase+merge in this case! Don't want to use up too much of everyone's valuable time trying to get things figured out. If we can't figure things out, just squashing is all right.

Aside: To allow a merge commit, I am going to hazard a guess that one setting to look at would be to see whether "Require linear history" is checked in https://github.com/exercism/problem-specifications/settings/branches (the protection rule for main). If it's checked, it would need to be unchecked to allow merge commits. I don't know if there are other settings to look at.

SaschaMann · 2022-03-30T19:23:27Z

I thought you were talking about a rebase merge. I thought we agreed that merge commits are not wanted at all some time ago?

ee7 · 2022-03-30T20:10:58Z

I thought we agreed that merge commits are not wanted at all some time ago?

I think the conclusion was #1934 (comment):

Okay. Having heard from a lot of people (thanks all!), let's make this a squash-only repository. If we ever want to merge/rebase, ping me and I can override it.

SaschaMann · 2022-03-30T20:20:14Z

No, I'm thinking of something older. Merge commits have (thankfully) been disabled across pretty much the entire org quite some time ago.

ErikSchierboom · 2022-03-31T06:41:34Z

@iHiD Could you rebase merge this?

bowling has `previousRolls` on a single line. transpose has each element of `lines` on its own line. pull request: #1968

format-array expresses our per-file preferences for array formatting, in cases where the preference differs from prettier. To see that this is true, the following procedure can be used: 1. Delete or truncate .pretterignore and run prettier on JSON files. prettier will have reformatted them. 2. run format-array. It will restore the files to their original state. pull request: #1968

pull request: #1968

with format-array running, we are now ready to allow prettier to format all. pull request: #1968

suggested as a follow-up action to format-array being merged, in at least two separate occasions: exercism#1968 (comment) exercism#1966 (comment)

suggested as a follow-up action to format-array being merged, in at least two separate occasions: exercism#1968 (comment) exercism#1966 (comment) The reason for the suggestion is that helps visualise the human-readable form of the triangle that we are used to seeing.

suggested as a follow-up action to format-array being merged, on at least two separate occasions: exercism#1968 (comment) exercism#1966 (comment) The reason for the suggestion is that helps visualise the human-readable form of the triangle that we are used to seeing.

suggested as a follow-up action to format-array being merged, on at least two separate occasions: #1968 (comment) #1966 (comment) The reason for the suggestion is that helps visualise the human-readable form of the triangle that we are used to seeing.

petertseng commented Feb 19, 2022

View reviewed changes

petertseng force-pushed the format-tool branch from 3e38ab1 to c384a84 Compare February 19, 2022 06:03

ErikSchierboom reviewed Feb 22, 2022

View reviewed changes

ErikSchierboom approved these changes Feb 25, 2022

View reviewed changes

petertseng force-pushed the format-tool branch 2 times, most recently from 6467919 to 95242c8 Compare March 4, 2022 12:57

petertseng force-pushed the format-tool branch from 95242c8 to 74a606c Compare March 4, 2022 20:25

petertseng marked this pull request as ready for review March 5, 2022 02:51

petertseng requested a review from a team as a code owner March 5, 2022 02:51

petertseng commented Mar 5, 2022

View reviewed changes

ErikSchierboom reviewed Mar 8, 2022

View reviewed changes

SaschaMann mentioned this pull request Mar 25, 2022

Add exercise: state-of-tic-tac-toe #1991

Merged

SaschaMann approved these changes Mar 29, 2022

View reviewed changes

iHiD approved these changes Mar 29, 2022

View reviewed changes

kotp approved these changes Mar 30, 2022

View reviewed changes

petertseng added 5 commits March 30, 2022 17:08

bowling, transpose: conform array format to rest of file

6cc77b6

bowling has `previousRolls` on a single line. transpose has each element of `lines` on its own line. pull request: exercism#1968

add format-array to CI

cfeef16

pull request: exercism#1968

rm prettierignore

ec78b41

with format-array running, we are now ready to allow prettier to format all. pull request: exercism#1968

CONTRIBUTING: mention format-array

4db2838

pull request: exercism#1968

petertseng force-pushed the format-tool branch from ab56243 to 4db2838 Compare March 30, 2022 17:13

iHiD merged commit b772d5f into exercism:main Mar 31, 2022

iHiD pushed a commit that referenced this pull request Mar 31, 2022

bowling, transpose: conform array format to rest of file

daf84d6

bowling has `previousRolls` on a single line. transpose has each element of `lines` on its own line. pull request: #1968

iHiD pushed a commit that referenced this pull request Mar 31, 2022

add format-array to CI

f84e129

pull request: #1968

iHiD pushed a commit that referenced this pull request Mar 31, 2022

rm prettierignore

2cb7634

with format-array running, we are now ready to allow prettier to format all. pull request: #1968

petertseng deleted the format-tool branch March 31, 2022 15:54

petertseng mentioned this pull request Mar 31, 2022

pascals-triangle: format each row as one line #1996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add format-array (formats JSON arrays) #1968

add format-array (formats JSON arrays) #1968

petertseng commented Feb 19, 2022 •

edited

Loading

petertseng commented Feb 19, 2022

petertseng Feb 19, 2022

SaschaMann Feb 19, 2022 •

edited

Loading

ErikSchierboom Feb 22, 2022

petertseng Feb 25, 2022

ErikSchierboom Feb 22, 2022

petertseng Feb 25, 2022

ErikSchierboom Feb 22, 2022

petertseng Feb 25, 2022

petertseng commented Feb 25, 2022

ErikSchierboom left a comment

petertseng commented Mar 4, 2022

petertseng commented Mar 4, 2022 •

edited

Loading

petertseng commented Mar 5, 2022 •

edited

Loading

petertseng Mar 5, 2022

petertseng commented Mar 5, 2022

petertseng commented Mar 5, 2022 •

edited

Loading

ErikSchierboom left a comment

ErikSchierboom commented Mar 29, 2022

petertseng commented Mar 29, 2022

ErikSchierboom commented Mar 30, 2022

iHiD commented Mar 30, 2022

ErikSchierboom commented Mar 30, 2022

SaschaMann commented Mar 30, 2022 •

edited

Loading

ErikSchierboom commented Mar 30, 2022

ee7 commented Mar 30, 2022 •

edited

Loading

petertseng commented Mar 30, 2022

SaschaMann commented Mar 30, 2022

ee7 commented Mar 30, 2022

SaschaMann commented Mar 30, 2022

ErikSchierboom commented Mar 31, 2022

add format-array (formats JSON arrays) #1968

add format-array (formats JSON arrays) #1968

Conversation

petertseng commented Feb 19, 2022 • edited Loading

petertseng commented Feb 19, 2022

Choose a reason for hiding this comment

SaschaMann Feb 19, 2022 • edited Loading

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

petertseng commented Feb 25, 2022

ErikSchierboom left a comment

Choose a reason for hiding this comment

petertseng commented Mar 4, 2022

petertseng commented Mar 4, 2022 • edited Loading

petertseng commented Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

petertseng commented Mar 5, 2022

petertseng commented Mar 5, 2022 • edited Loading

ErikSchierboom left a comment

Choose a reason for hiding this comment

ErikSchierboom commented Mar 29, 2022

petertseng commented Mar 29, 2022

ErikSchierboom commented Mar 30, 2022

iHiD commented Mar 30, 2022

ErikSchierboom commented Mar 30, 2022

SaschaMann commented Mar 30, 2022 • edited Loading

ErikSchierboom commented Mar 30, 2022

ee7 commented Mar 30, 2022 • edited Loading

petertseng commented Mar 30, 2022

SaschaMann commented Mar 30, 2022

ee7 commented Mar 30, 2022

SaschaMann commented Mar 30, 2022

ErikSchierboom commented Mar 31, 2022

petertseng commented Feb 19, 2022 •

edited

Loading

SaschaMann Feb 19, 2022 •

edited

Loading

petertseng commented Mar 4, 2022 •

edited

Loading

petertseng commented Mar 5, 2022 •

edited

Loading

petertseng commented Mar 5, 2022 •

edited

Loading

SaschaMann commented Mar 30, 2022 •

edited

Loading

ee7 commented Mar 30, 2022 •

edited

Loading