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

style: simplify formatting with deno, part 2: JSON #3118

Closed
wants to merge 2 commits into from

Conversation

scarf005
Copy link
Member

@scarf005 scarf005 commented Sep 6, 2023

Summary

SUMMARY: Infrastructure "Simplified formatting all JSONs with deno"

Purpose of change

part 1 (#3108) formatted all markdown files with deno. and deno can format JSONs.

Describe the solution

  • removed tools/format.
  • removed all references to tools/format, in cmake, makefile, python tools, and documentation.
  • formatted all JSON files with deno.
  • updated relevent docs.

after the merge, a new PR with .git-blame-ignore-revs will be added to make navigating git blames easier, such as:

# Run this command to always ignore formatting commits in `git blame`
# git config blame.ignoreRevsFile .git-blame-ignore-revs

# See: <https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view>

# formatted all markdown files with deno
adeda638f33130cb1fb454def0067b329ced17fe

# formatted all JSON files with deno
4faf34d1d12698fe28473d6fdd72bdbec18979b4

Additional context

needs to be rebase merged in order to only ignore formatting commit.

@github-actions github-actions bot added data PRs related to datas. Won't crash game (probably) mods PR changes related to mods. labels Sep 6, 2023
@scarf005 scarf005 force-pushed the json-fmt branch 3 times, most recently from aa00fb6 to e9b3f9e Compare September 6, 2023 05:12
@scarf005 scarf005 marked this pull request as ready for review September 6, 2023 06:07
@olanti-p
Copy link
Member

olanti-p commented Sep 29, 2023

To reiterate on conversation from Discord:

This is a huge change, affecting almost 2600 files. Such blanket changes make it extremely hard to cherry-pick code from DDA, as every changed line results in a new merge conflict that could've been avoided.

Unfortunately deno fmt is not flexible enough when it comes to JSON, it's impossible to configure it to decrease the amount of changes caused by switching to it.

It is possible to have a custom deno task that goes something like deno run fmt that would download our precompiled json formatter from somewhere and and run it on all (or modified) JSONs, but that may not work nicely with the deno language server.

This does not pose an issue for VSCode, however, since the formatter is already available as an extension that was originally developed for DDA but is also compatible with BN JSON style:
https://marketplace.visualstudio.com/items?itemName=cdda-toys.cdda-json-formatter
The extension provides format-on-save functionality, and for other IDEs it is usually possible to set up custom run-on-save commands that would call the formatter with proper arguments.

I don't think us using custom JSON formatter is bad in on itself. It does make cross-platform support harder, but not impossible. It works fine on Linux/Windows/MacOS, and we're not currently interested in other targets like WASM (and for Web world, the json formatter supports CGI interface).

@scarf005 scarf005 closed this Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data PRs related to datas. Won't crash game (probably) mods PR changes related to mods.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants