Skip to content

cmd/shfmt: implement --from-json and improve -tojson#900

Merged
mvdan merged 1 commit into
masterfrom
from-json-3
Jul 16, 2022
Merged

cmd/shfmt: implement --from-json and improve -tojson#900
mvdan merged 1 commit into
masterfrom
from-json-3

Conversation

@mvdan

@mvdan mvdan commented Jul 13, 2022

Copy link
Copy Markdown
Owner

(see commit message)

Fixes #35 again, as we never implemented the "read JSON" side.

@mvdan

mvdan commented Jul 13, 2022

Copy link
Copy Markdown
Owner Author

cc @cilki @JounQin

@mvdan mvdan requested a review from riacataquian July 13, 2022 16:26
@mvdan

mvdan commented Jul 13, 2022

Copy link
Copy Markdown
Owner Author

A large portion of the diff is JSON output in test files, which was automatically generated via go test -u. It shouldn't be closely reviewed, but I did skim it for any mistakes or unexpected changes.

@riacataquian riacataquian left a comment

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.

neat :) LGTM!

Comment thread cmd/shfmt/main.go
@JounQin

JounQin commented Jul 15, 2022

Copy link
Copy Markdown
Contributor

So it won't a core API but left in shfmt right? I'd like to use it without shfmt for small bundle, should I copy/paste related codes instead?

@mvdan

mvdan commented Jul 15, 2022

Copy link
Copy Markdown
Owner Author

@JounQin One step at a time :) If I was implementing from-json, removing reflect, and adding the library API all in one PR, this would take weeks and be an absolutely massive change.

@JounQin

JounQin commented Jul 15, 2022

Copy link
Copy Markdown
Contributor

@mvdan Thanks for clarifying, I just want to make sure it will be in core API in the future.

@mvdan

mvdan commented Jul 16, 2022

Copy link
Copy Markdown
Owner Author

Did one last push as I forgot to update the man page; see https://github.com/mvdan/sh/compare/46e0c4e09353c070bc3e23b1aac4a3b800c64d4b..e5d80446cfdede8b096dd7c9553edf1e73ce5ab7. I'll be merging on green. Thanks for the review!

For the sake of consistency, -tojson is now also available as --to-json.

The following changes are made to --to-json:

1) JSON object keys are no longer sorted alphabetically.
   The new order is: the derived keys (Type, Pos, End),
   and then the Node's struct fields in their original order.
   This helps consistency across nodes and with the Go documentation.

2) File.Name is empty by default, rather than `<standard input>`.
   It did not make sense as a default for emitting JSON,
   as the flag always required the input to be stdin.

3) Empty values are no longer emitted, to help with verbosity.
   This includes `false`, `""`, `null`, `[]`, and zero positions.
   Positions offsets are exempt, as 0 is a valid byte offset.

4) All position fields in Node structs are now emitted.
   Some positions are redundant given the derived Pos and End keys,
   but some others are entirely separate, like IfClause.ThenPos.

As part of point 1 above, JSON encoding no longer uses Go maps.
It now uses reflect.StructOf, which further leans into Go reflection.

Any of these four changes could potentially break users,
as they slightly change the way we encode syntax trees as JSON.
We are working under the assumption that the changes are reasonable,
and that any breakage is unlikely and easy to fix.
If those assumptions turn out to be false once this change is released,
we can always swap the -tojson flag to be exactly the old behavior,
and --to-json can then add the new behavior in a safer way.

We also had other ideas to further improve the JSON format,
such as renaming the Type and Pos/End JSON keys,
but we leave those as v4 TODOs as they will surely break most users.

The new --from-json flag does the reverse; it decodes the typed JSON,
and fills a *syntax.File with all the information.
The derived Type field is used to select syntax.Node types,
and the derived Pos and End fields are ignored entirely.

It's worth noting that neither --to-json nor --from-json are optimized.
The decoding side first decodes into an empty interface, for example,
which leaves plenty of room for improvement.
Once we're happy with the functionality, we can look at faster
implementations and even dropping the need for reflection.

While here, I noticed that the godoc for Pos.IsValid was slightly wrong.

As a proof of concept, the following commands all produce the same result:

	shfmt <input.sh
	shfmt --to-json <input.sh | shfmt --from-json
	shfmt --to-json <input.sh | jq | shfmt --from-json

Fixes #35 again, as we never implemented the "read JSON" side.
@mvdan mvdan merged commit c016564 into master Jul 16, 2022
@mvdan mvdan deleted the from-json-3 branch July 16, 2022 22:34
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.

cmd/shfmt: add flags to read/write AST in JSON format to simplify integration with other languages

3 participants