dlang / dub

Package and build management system for D
MIT License
675 stars 227 forks source link

How do I put comment in JSON now? #2567

Open p0nce opened 1 year ago

p0nce commented 1 year ago

System information

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

veelo commented 1 year ago

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

p0nce commented 1 year ago

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

Geod24 commented 1 year ago

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 commented 1 year ago

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

Geod24 commented 1 year ago

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 commented 1 year ago

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

p0nce commented 1 year ago

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 commented 1 year ago

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

p0nce commented 1 year ago

Many reasons:

CyberShadow commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

p0nce commented 1 year ago

@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 commented 1 year ago

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)