Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat: 🎸 allow comments in json file #4382

Closed

Conversation

IWANABETHATGUY
Copy link
Contributor

Summary

Test Plan

Changelog

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Apr 17, 2023

Deploy Preview for docs-rometools canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 3b9a9b9
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/6455434e9a63c2000886d2a2

@github-actions github-actions bot added A-Project Area: project configuration and loading A-Parser Area: parser labels Apr 17, 2023
@github-actions github-actions bot removed the A-Parser Area: parser label May 3, 2023
@github-actions github-actions bot added A-CLI Area: CLI A-Formatter Area: formatter A-Parser Area: parser A-Tooling Area: our own build, development, and release tooling labels May 3, 2023
@IWANABETHATGUY
Copy link
Contributor Author

IWANABETHATGUY commented May 4, 2023

https://github.com/IWANABETHATGUY/tools/blob/ab690e1f6e9877c2c17a330095faf4e9b2a13892/crates/rome_service/src/settings.rs#L198-L207, @ematipico sorry for the ping, I am stuck now, I want to pass the JsonConfiguration.allow_comments to LanguageSettings, but Language trait now is hard to pass language-specific feature, like allowComments options, would you mind I modify the trait and add another associated field allow_comments ?

@ematipico
Copy link
Contributor

IWANABETHATGUY/tools@ab690e1/crates/rome_service/src/settings.rs#L198-L207, @ematipico sorry for the ping, I am stuck now, I want to pass the JsonConfiguration.allow_comments to LanguageSettings, but Language trait now is hard to pass language-specific feature, like allowComments options, would you mind I modify the trait and add another associated field allow_comments ?

Sure, it makes sense!

Since each language section is divided by "tool", I think here we should do the same have something like this:

{
	"json": {
		"parser": {
			"allowComments": true
		}
	}
}

What do you think?

@ematipico
Copy link
Contributor

@IWANABETHATGUY are there any updates? do you need any help?

@ematipico
Copy link
Contributor

Closing in favour of #4714

@ematipico ematipico closed this Jul 20, 2023
@IWANABETHATGUY IWANABETHATGUY deleted the feat/allow-comments-in-json branch July 20, 2023 09:08
kodiakhq bot pushed a commit to vercel/turborepo that referenced this pull request Aug 14, 2023
When we run `link` and `unlink` we want those commands to create the
smallest set of changes possible in the output while also accounting for
edge cases like multiple definition.

This PR adds an AST-aware JSON-rewriter that modifies the file contents
rather than attempting to deserialize and reserialize the entire
document (which would otherwise be lossy unless deserialized to a
concrete syntax tree).

The only implementation of a concrete syntax tree for JSON in the Rust
ecosystem appears to be
https://github.com/rome/tools/tree/main/crates/rome_json_parser which
presently lexes `jsonc`, but does not link it into the rest of the
parser. There is an open, abandoned PR for that effort:
rome/tools#4382

After review, I decided that the additional complexity and size of
pulling in the entire Rome parsing toolchain was not going to be
worthwhile at this time for a task this small in scope.

I investigated `pest` as an alternative, but grammar correctness edge
cases for JSON are not great, and the whitespace and comment macros in
the `pest` grammar would have to be hand-implemented. I elected against
this for low confidence in a toy parser.

Eventually I chose to use `jsonc_parser` which doesn't actually create
all of the necessary tokens, but using conservative patterns on top of
the limited information I can ensure that it doesn't _fail_. However,
this greatly limits the formatting abilities that we can accomplish. I
decided this was an acceptable tradeoff for complexity, total effort,
dep size, scope of use (just `link` and `unlink` setting one property).

Additional output formatting improvements are out-of-scope for this PR.
This generates assuredly parseable content, but makes no guarantees as
to what the output format looks like.

---------

Co-authored-by: Nathan Hammond <Nathan Hammond>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI A-Formatter Area: formatter A-Parser Area: parser A-Project Area: project configuration and loading A-Tooling Area: our own build, development, and release tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants