Skip to content

Format canonical data using jq#1916

Closed
ErikSchierboom wants to merge 1 commit intomainfrom
format
Closed

Format canonical data using jq#1916
ErikSchierboom wants to merge 1 commit intomainfrom
format

Conversation

@ErikSchierboom
Copy link
Copy Markdown
Member

@ErikSchierboom ErikSchierboom commented Jan 11, 2022

While looking into re-ordering the keys across all canonical-data.json files, I found that there was a lot of inconsistency in the formatting between files.
This isn't a big problem, but some consistency is nice.
For this draft PR, I've used jq to reformat all the canonical data files, as its style of formatting seemed to be slightly closer to our current style than prettier.

The goal of this file is to check which formatting changes are desirable, and more important: which are not.

Copy link
Copy Markdown
Contributor

@SaschaMann SaschaMann left a comment

Choose a reason for hiding this comment

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

Exploding arrays onto several lines seems undesirable in most cases.

"property": "accumulate",
"input": {
"list": [1, 2, 3],
"list": [
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These don't seem desirable. They make reading and copy and pasting cases harder when manually implementing a solution.

Comment on lines +90 to +104
[
"a1",
"a2",
"a3"
],
[
"b1",
"b2",
"b3"
],
[
"c1",
"c2",
"c3"
]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is way harder to read than the matrix on the left.

Comment on lines +70 to 78
"digits": [
1,
0,
1,
0,
1,
0
],
"outputBase": 10
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't read like a binary number anymore.

"The error object is used to indicate that the value is not included in the array.",
"It should be replaced with the respective expression that is idiomatic",
"for the language that implements the tests.",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should probably be replaced with a "", or ignored then because the newline seems intentional.

Comment on lines +81 to +94
"dominoes": [
[
1,
2
],
[
3,
1
],
[
2,
3
]
]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

An even worse example than the ones above 😅

@ErikSchierboom
Copy link
Copy Markdown
Member Author

Totally agree on the array examples. Are all arrays rewrites bad, or just some?

@SaschaMann
Copy link
Copy Markdown
Contributor

SaschaMann commented Jan 11, 2022

I've only skimmed and commented on a few. I think for the inputs/outputs, they might not all be that bad but manual formatting depending on the context would be better.

As for reformatting the comment arrays, that seems pointless but also not harmful.

"square": 64
},
"expected": 9223372036854775808
"expected": 9223372036854776000
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unfortunate. for this reason, we may not be able to use jq to format grains.

"property": "total",
"input": {},
"expected": 18446744073709551615
"expected": 18446744073709552000
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unfortunate. for this reason, we may not be able to use jq to format grains.

@ErikSchierboom
Copy link
Copy Markdown
Member Author

ErikSchierboom commented Jan 12, 2022

I've also opened #1917 to see what the formatting looks like when using prettier.

The prettier changes look more sensible to me.

@ErikSchierboom
Copy link
Copy Markdown
Member Author

Closing this in favor of #1917

@ErikSchierboom ErikSchierboom deleted the format branch January 13, 2022 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants