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

How do I put comment in JSON now? #2567

Open
p0nce opened this issue Jan 17, 2023 · 20 comments
Open

How do I put comment in JSON now? #2567

p0nce opened this issue Jan 17, 2023 · 20 comments

Comments

@p0nce
Copy link
Contributor

p0nce commented Jan 17, 2023

System information

  • dub version: DUB version 1.31.0-rc.1, built on Jan 17 2023
  • OS Platform and distribution: Windows 11
  • compiler version LDC 1.30.0

Bug Description

DUB complains about non-existent keys in the schema, even those called "comment".
But those JSON comments are there to avoid mistakes for the users of the project.

How to reproduce?

Build this project => https://github.com/AuburnSounds/Dplug/tree/master/examples/ms-encode who has a "comment-stuff" key.
Keys named just "comment" could be in the schema, and ignored.

Expected Behavior

A special way to write comment in dub.json, like we were advised to do. I don't want to go .sdl, it would disrupt users more.

Logs

     Warning C:\Users\guill\Desktop\Dplug\examples\ms-encode\dub.json(15:4): comment: Key is not a valid member of this section. There are 53 valid keys: name, description, homepage, authors, copyright, license, toolchainRequirements, configurations, buildTypes, -ddoxFilterArgs, -ddoxTool, subPackages, version, dependencies, systemDependencies, targetType, targetPath, targetName, workingDirectory, mainSourceFile, subConfigurations, dflags, lflags, libs, sourceFiles, sourcePaths, excludedSourceFiles, injectSourceFiles, copyFiles, extraDependencyFiles, versions, debugVersions, versionFilters, debugVersionFilters, importPaths, stringImportPaths, preGenerateCommands, postGenerateCommands, preBuildCommands, postBuildCommands, preRunCommands, postRunCommands, environments, buildEnvironments, runEnvironments, preGenerateEnvironments, postGenerateEnvironments, preBuildEnvironments, postBuildEnvironments, preRunEnvironments, postRunEnvironments, buildRequirements, buildOptions
@WebFreak001
Copy link
Member

I think we could go with something like reserving underscore prefixed keys to be always comments - thoughts @Geod24 ?

On the other hand we could also start supporting regular JS-style comments. However right now there are most likely development apps reading dub.json files manually to look for certain keys, so we wouldn't want to break those. To properly read what the DUB recipe does you should rather use dub describe or at least dub convert, but both of those are still way too slow, taking up possibly several hundred milliseconds - so it's not feasible to expect users to use them in all cases right now.

Personally I think JS-style comments would be the most intuitive and easy to understand form inside JSON files. However I think the slowness of dub convert and dub describe is what I would consider a blocker for this feature. (as well as, of course, the dub registry also needing to support it)

@p0nce
Copy link
Contributor Author

p0nce commented Jan 17, 2023

both of those are still way too slow, taking up possibly several hundred milliseconds - so it's not feasible to expect users to use them in all cases right now.

Yes, that's indeed what happens here, in addition. The comments here are intended for people copying the recipe from examples.
(btw the newer DUB is amazing with the builtin schema and colors!)

@Geod24
Copy link
Member

Geod24 commented Jan 18, 2023

I think underscores (or some other prefix) makes sense as a stopgap measure. As a long term solution, either JSON5 or YAML ? :)

@veelo
Copy link
Contributor

veelo commented Jan 19, 2023

Can we add comment to the list? This is probably very common, and changing those to _comment is silly.

@p0nce
Copy link
Contributor Author

p0nce commented Jan 19, 2023

I would also want a "comment" key to be accepted. :) this can be nice semantic information for displaying dub.json also eventually

@Geod24
Copy link
Member

Geod24 commented Jan 20, 2023

One of the issue I have with comment is that it can only be used once. The new dub parser will issue a warning on duplicated keys and fall back to the old parser (which does not check fields).

@veelo
Copy link
Contributor

veelo commented Jan 20, 2023

So if I have more than one comment in a json, i have to call them _comment1, _comment2 etc? I hope not...

@Geod24
Copy link
Member

Geod24 commented Jan 20, 2023

I need to see, the error currently comes from the YAML parser, it's not something we control. But no, we're just going to flat out ignore _ fields.

@veelo
Copy link
Contributor

veelo commented Jan 20, 2023

Wouldn't it be possible to also flat out ignore comment fields?

@p0nce
Copy link
Contributor Author

p0nce commented Jan 21, 2023

Personally I can live with _comment, it's also a solution if someone really wants to extend dub.json format for their build (a bad idea, I learned)

@katyukha
Copy link

katyukha commented May 5, 2023

Why not use just dub.sdl - it is much more readable and support comments? :)

@p0nce
Copy link
Contributor Author

p0nce commented May 15, 2023

Many reasons:

  • the developer experience has regressed. You used to be able to put comment in dub.json, now it's a massive error message
  • developers come to Dplug without knowing D programming, audio plugins programming, and DUB. They must learn these 3 things at the same time, so it's nice not to force them to learn another language they will only encounter there (in 30 lines files, moreover). DUB is not the easiest thing to learn, and this is accidental
  • going SDL has solved no big problem for the D community

@CyberShadow
Copy link
Member

I think at some point Dub started using a YAML parser for dub.json. YAML is a superset of JSON so existing files "just work".

But this also means you can now just use YAML syntax, including comments. Right?

@p0nce
Copy link
Contributor Author

p0nce commented May 31, 2023

Well YAML is not a superset, since multiple keys with same name do not parse.

The problem is we have a build tool (necessary to do Universal Builds on macOS and much other stuff) that read some keys from dub.json. It does it using std.json parser. dub describe was too slow for this. I can't be the only one that implements dub.json based tooling.

This changes pushed me to have the build tool include the (relatively complicated and less maintained) sdlang-d parser, so we will be able to move to SDLang (and not by choice). Comments are not a big issue, but still the repercussion of the decision to go SDLang instead of not doing anything never quite stops. I'm not quite ready to add a third parser.

@CyberShadow
Copy link
Member

I think Dub should be the only software parsing Dub configuration files. Trying to do it any other way is where the problems start. If we start saying "we can't change dub.sdl/dub.json because it will break p0nce's parser", it will be the start of the end of days :)

If it can't use dub describe, perhaps your tool could use Dub as a library. Would that work? If not then I think Dub should be refactored to allow this.

@WebFreak001
Copy link
Member

there are a bunch of tools that parse dub.json, although the recommended approach to parse the dub.json file (and dub.sdl) is by parsing the output of dub convert --format=json, which also works with json input. However the problem with this is that it's super slow since it does all the dub startup stuff.

@rikkimax
Copy link
Contributor

Editors like IntelliJ parse and build a tree that you can modify for JSON files, including for dub.json.

We shouldn't be introducing a new syntax feature for documented file formats like JSON.

@CyberShadow
Copy link
Member

OK, but therefore we should not have made Dub use a YAML parser for dub.json, right?

@p0nce
Copy link
Contributor Author

p0nce commented May 31, 2023

@CyberShadow first of all, that's a bit much, it was said when SDLang was introduced that JSON would always be supported.
And that you could use comments with "comment" key. And now, valid JSON is rejected :) so break of given word.

Anyway, because the issue is small, I think a good way would just be an escape hatch like "_comment1" in the YAML parser.
(The first thing users do when they come to a D framework is duplicate an example and keep the path-based dependencies. So the example break. The comments are here to make them understand how to use ~> instead.)

I'm a bit wary for frequently duplicated keys, like "versions". Need to test that. (EDIT: it's better now, multiple "versions" was silently failing in the first place)

EDIT: Other than that, the new YAML parser does manages the schema internally! It is well worth it.

@p0nce
Copy link
Contributor Author

p0nce commented May 31, 2023

Former DUB doesn't warn on multiple "versions" keys, but the new DUB does. And the new one is more correct, since the second key would silently erase the first in former DUB. So this dupe key case was much improved by the move to single-keys.

"versions": ["a"],
"versions": ["b"],

New message:

Warning Error was: dub.internal.dyaml.composer.ComposerException@source\dub\internal\dyaml\composer.d(370): Key 'versions' appears multiple times in mapping (first: file C:\Users\guill\Desktop\Dplug\examples\distort\dub.json,line 13,column 5)

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

No branches or pull requests

7 participants