Skip to content
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

consistent JSON formatting of schemas? #4072

Open
rogpeppe opened this issue Sep 13, 2024 · 5 comments
Open

consistent JSON formatting of schemas? #4072

rogpeppe opened this issue Sep 13, 2024 · 5 comments
Labels
issue:bug Report errors or unexpected behavior (auto-generated by issue forms) question

Comments

@rogpeppe
Copy link
Contributor

Description of the feature / enhancement.

Some of the issues (#3102 holding some examples) could potentially be addressed by an automated change that reads each schema, applies some fix, and writes it back out again. However, that kind of automated change is awkward to review if the round-trip results in a complete reformatting of the file, making the important part of the diff hard to see.

I may have missed something (I hope so) but ISTM that if all the JSON files were formatted in a consistent tooling-friendly way, then such changes would be straightforward.

As a straw man suggestion, how about running all the files through jq . ? Then a user can always do the same thing before committing.

If there is such a formatting standard in place, then perhaps it could be documented in https://github.com/SchemaStore/schemastore/blob/master/CONTRIBUTING.md.

Are you making a PR for this?

No, someone else must create the PR.

@hyperupcall
Copy link
Member

hyperupcall commented Sep 16, 2024

Yes! We use prettier to format the schemas. You can run Prettier locally with npm run prettier and run fixes with npm run prettier:fix. We have a little bit of configuration to put things like id, $id, $ref, etc. near the top of object types.

However, we do have an issue where pre-commit behaves weirdly with files paths like src/json/prettier/.prettierrc.json, src/json/prettier/prettier.yaml - so don't commit those if those are formatted. Once I get around to replacing pre-commit.ci with GitHub CI, that should no longer be an issue.

@rogpeppe
Copy link
Contributor Author

We have a little bit of configuration to put things like id, $id, $ref, etc. near the top of object types

Ah, that's super interesting, thanks!
Out of interest, perhaps you could explain how it works? I'm looking at:

          $schema: null,
          $id: null,
          $comment: null,
          $ref: null,
          '/^\\$.*/': null,
          '/^[^\\d+]/': 'none',
          '/^\\d+/': 'none',
          if: null,
          then: null,
          else: null,
          '/^[^\\d+]/': 'none',
          '/^\\d+/': 'none',

and I'm wondering why some of the rules are duplicated, and why there are special rules for keys that start with a digit or a plus sign.
FWIW '/^[^\\d+]/' rule looks like to me the + in it might be a copy/paste mistake from the '/^\\d+/' rule above it.

I did a quick grep and while there are object keys that start with a digit, they're often the start of a hex string, so having a special case for a leading digit might lead to odd ordering with respect to other similar hex keys that happen to start with a letter.

But I might well be misunderstanding the configuration based on a very quick scan of https://www.npmjs.com/package/prettier-plugin-sort-json (which also seems to contain the same /^[^\d+]/ idiom, so perhaps that's where it comes from, although I'm fairly sure it doesn't work the way it's intended there, as unless it's using a different kind of regex engine, a + inside a character class just means a literal + character).

@hyperupcall hyperupcall added question issue:bug Report errors or unexpected behavior (auto-generated by issue forms) labels Sep 19, 2024
@hyperupcall
Copy link
Member

hyperupcall commented Sep 19, 2024

and I'm wondering why some of the rules are duplicated, and why there are special rules for keys that start with a digit or a plus sign.
FWIW '/^[^\\d+]/' rule looks like to me the + in it might be a copy/paste mistake from the '/^\d+/' rule above it.

Yeah looking back at the configuration, I'm noticing some of the same issues as well. The duplicated rules before and after the { "if": null, "then": null, "else": null } are supposed to explicitly mean that there is no sorting before and after the properties with if, then, and else. But I'm not sure that properly translates since objects in JavaScript have unique keys so only the "bottom properties" probably show up in the object.

I believe there are two rules that start with digit and plus because they are both mean to disable the default formatting of numbers and "words". Numbers default to sorting by order and words default to sorting by lexicographic, so I think they each have to be reset individually.

I did a quick grep and while there are object keys that start with a digit, they're often the start of a hex string, so having a special case for a leading digit might lead to odd ordering with respect to other similar hex keys that happen to start with a letter.

That makes sense, I didn't think of that case when making the rules.

But I might well be misunderstanding the configuration based on a very quick scan of npmjs.com/package/prettier-plugin-sort-json (which also seems to contain the same /^[^\d+]/ idiom, so perhaps that's where it comes from, although I'm fairly sure it doesn't work the way it's intended there, as unless it's using a different kind of regex engine, a + inside a character class just means a literal + character).

Ohh you're right, the + is misplaced. It should definitely be a bug, I just copied it from prettier-plugin-sort-json.

Thanks for your feedback! The configuration can definitely use some improvement by working against these additional cases

@rogpeppe
Copy link
Contributor Author

One thing I don't understand: AFAICS the prettier configuration seems to be set up to sort all object properties recursively. But to take an arbitrary example, when I look at the keys in #/definitions in github-workflow.json, they don't appear to be sorted, and prettier doesn't do anything when I run it against that file. What am I missing?

@rogpeppe
Copy link
Contributor Author

rogpeppe commented Oct 14, 2024

FWIW I experimented by removing the entire jsonSortOrder key from the src/schemas/json/** section in .prettierrc.cjs (but leaving jsonRecursiveSort: true in place), then ran npm run prettier again and it did not result in any warnings, which makes me concerned that perhaps this configuration isn't actually doing what it's intended to be doing.

That said, I'm not at all familiar with the Node ecosystem, so I may well have installed dependencies incorrectly or be running it wrongly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue:bug Report errors or unexpected behavior (auto-generated by issue forms) question
Projects
None yet
Development

No branches or pull requests

2 participants