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

Add fix-byte-order-marker and pretty-format-json to pre-commit #2634

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

dekkers
Copy link
Contributor

@dekkers dekkers commented Mar 12, 2024

Changes

This pretty formats all JSON files. The reason is I saw the whitespace changes in #2556 which only causes unnecessary conflicts when there are other changes. It will also help the JSON files in #2545 to be better readable automatically and consistently formatted.

I added --no-ensure-ascii because as far as I know there is no reason to escape utf-8 characters in JSON files.

This also adds fix-byte-order-marker because keiko/glossaries/glossary-export.json had a byte order mark which is unnecessary and not recommended for UTF-8 but caused pretty-format-json to give an error.


Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@dekkers dekkers requested a review from a team as a code owner March 12, 2024 21:55
@dekkers dekkers self-assigned this Mar 12, 2024
@underdarknl
Copy link
Contributor

I agree with --no-ensure-ascii and the other utf8 related fixes.

Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I prefer to also use --no-sort-keys, because order of properties may also have semantic meaning and currently without that option it produces a lot of changes.

@dekkers
Copy link
Contributor Author

dekkers commented Mar 18, 2024

Looks good to me. I prefer to also use --no-sort-keys, because order of properties may also have semantic meaning

I saw that we tried to order some files in a certain way, but JSON is unordered and that makes it harder to enforce a non-lexicographical order. For example do the environment_keys in the boefje.json need to be specified before or after the scan_level? One boefje does it differently than the other boefjes, are we going to document this currently undocumented ordering and put time into "fixing" this?

and currently without that option it produces a lot of changes.

It only produces a lot of changes once. After that there is a strict order and it is automatically fixed. There will also be no noise in PRs if things accidentally move around.

@ammar92
Copy link
Contributor

ammar92 commented Mar 18, 2024

Looks good to me. I prefer to also use --no-sort-keys, because order of properties may also have semantic meaning

I saw that we tried to order some files in a certain way, but JSON is unordered and that makes it harder to enforce a non-lexicographical order. For example do the environment_keys in the boefje.json need to be specified before or after the scan_level? One boefje does it differently than the other boefjes, are we going to document this currently undocumented ordering and put time into "fixing" this?

I understand that JSON properties are unordered, but what does it exactly "fix" by messing around with (primarily) human written manifests, test stubs and schemas?

And they actually more or less follow the order as defined in the respective Python model. And for me it makes more sense to have some expectation in ordering. Instead of fixating on enforcing a rigid lexicographical order, we should prioritize readability and logical organization within the JSON files. This means having some level of expectation in ordering, such as placing essential identifiers like ID or title at the top followed by logically grouped properties such as consumes and produces. It doesn't matter if the order isn't strict, who cares if scan_level or environment_keys is two or three positions "off"? I don't think these minor order variations are problematic and worthy of putting a significant time and effort into fixing this.

and currently without that option it produces a lot of changes.

It only produces a lot of changes once. After that there is a strict order and it is automatically fixed. There will also be no noise in PRs if things accidentally move around.

The noise is usually whitespace and line endings. I understand that there will be minor changes after this, but I'm still struggling to see how reordering the properties (at least for non-generated documents) benefits us in the first place.

@dekkers dekkers force-pushed the pretty-format-json branch from b623706 to 2aee8c3 Compare March 20, 2024 10:19
@dekkers
Copy link
Contributor Author

dekkers commented Mar 20, 2024

I have added --no-sort-keys and ran pre-commit again on the original files.

@underdarknl underdarknl merged commit b0c97dd into main Mar 22, 2024
32 checks passed
@underdarknl underdarknl deleted the pretty-format-json branch March 22, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants