Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Read JSON-like files with comments inside #525

Closed
dpashkevich opened this Issue Nov 14, 2012 · 18 comments

Comments

Projects
None yet
7 participants

Ok, technically that wouldn't be JSON anymore but some tools (SublimeText 2, jshint) use configuration files with JSON syntax but also allow comments inside them. I think it's very useful!

You can see how jshint handles this here:
https://github.com/jshint/jshint/blob/master/src/cli/cli.js#L78
They simply strip the comments before actually "reading JSON".

Perhaps we can add a flag to grunt.file.readJSON() ? E.g.

grunt.file.readJSON('.jshintrc', true);   // true to strip comments

The reason I think this functionality needs to be exposed under a flag is because sometimes you might still want to enfore "strict" JSON validation.

Owner

cowboy commented Nov 14, 2012

"JSON with comments" isn't JSON. It's something else.

I know, see my first phrase :)

But let's be practical here:

  • JSON format is widely used for configuration files
  • comments are widely used in configuration files

Can we make make a common practice little easier? When a piece of configuration gets big or when it can be used by other tools besides grunt, I want to bring it outside of the gruntfile and then just do grunt.file.readJSON(). I use this quite often (see https://gist.github.com/3851841 as one of the examples) but I feel so constrained because I can't put comments in those external files.

Fine, we shouldn't call it JSON anymore (I edited the issue title btw :)) but I'm just saying it's useful. And grunt is a tool that helps people do common things, that's what makes grunt better than bare makefiles or quick nodejs scripts.
I think my suggestion complies with your philosophy:

The intention for plugins and proprietary tasks is that they are simply a grunt-compatible wrapper around an existing lib. For example, the built-in lint task is a wrapper around the node-jshint lib. Of course, you can use that lib programatically in standard Node.js scripts, but if you use the grunt task you get grunt-style logging, consistent file globbing, etc.

Imagine ST2 config files without the comments - nightmare!

Owner

cowboy commented Nov 14, 2012

Can you ask the maintainer of node-jshint to expose this functionality? Or to at least use a common lib for it that we can also use?

Owner

cowboy commented Nov 14, 2012

(I'm getting this and gruntjs/grunt-contrib-jshint#2 confused in my brain)

Owner

tkellen commented Nov 14, 2012

Looks like https://github.com/getify/JSON.minify would do the trick, but it isn't an NPM module.

Owner

tkellen commented Nov 14, 2012

Have you tried using YAML for your configuration files? It supports comments, looks better, and is supported by like, every programming language. JSON is a data interchange format, it's not really meant for humans to read and write.

Owner

cowboy commented Nov 14, 2012

FWIW, JSON is meant to be a data interchange format. It's not really designed to be authored directly, and in that context, comments really make no sense.

I'd suggest authoring your configuration files in YAML which is supported (in grunt 0.4.0+ at least) via the grunt.file.readYAML method. YAML supports comments, multi-line strings, automatic parsing of dates and more.

Owner

cowboy commented Nov 14, 2012

Haha, you beat me to it!

(I'm getting this and gruntjs/grunt-contrib-jshint#2 confused in my brain)

@cowboy me too now :) It all started with me installing the SublimeLinter plugin and trying to make it share the jshint configuration with grunt. I learned about .jshintrc and was pleased to find out that I can put comments into that file. So I thought maybe that would be very useful outside of jshint's scope (and besides, as noted, ST2 itself uses JSON-like configuration file syntax) so I filed a more general issue here. Sorry for all the trouble :)

@cowboy, @tkellen Thanks for suggesting YAML (and great that grunt now supports readYAML), that may work too, I guess it's a question of style. It just sounds simpler to use the syntax I'm already comfortable with, that's why people "abuse" the good ol' JSON :) Btw, I was surprised to find out that YAML doesn't have block comments...

Owner

shama commented Nov 14, 2012

JSON5 was also suggested on #222 which would allow comments.

Owner

cowboy commented Dec 31, 2012

Closing this. If you want to read "wacky" JSON files, you can use whatever node lib you'd like. JSON5 seems to do what you want.

@cowboy cowboy closed this Dec 31, 2012

mehcode commented Dec 31, 2012

Closing this. If you want to read "wacky" JSON files, you can use whatever node lib you'd like. JSON5 seems to do what you want.

Just as a fun side note. JSON is YAML. If you take straight JSON and just add # comments it can be parsed properly by a YAML parser.

Owner

cowboy commented Dec 31, 2012

Yes, JSON is a subset of YAML.

onury commented Jul 29, 2014

You can use the strip-json-comments package or similar and do:

var fs = require('fs'),
    stripJSONComments = require('strip-json-comments');
function readJSON(jsonFile) {
    var data = fs.readFileSync(jsonFile, 'utf8');
    return JSON.parse(stripJSONComments(data));
}
Owner

cowboy commented Jul 30, 2014

Member

sindresorhus commented Jul 30, 2014

@cowboy Even though you can use JSON syntax in YAML with comments. YAML does come with a lot of baggage, overhead, security issues and is much slower than stripping out comments from JSON and parsing it with JSON.parse. I don't personally care, but it's easier to just add this rather than trying to convince everyone YAML with JSON syntax to be the way to go. Just a thought.

onury commented Jul 30, 2014

Well, I'm half-way convinced. I've decided to use YAML for my Grunt config files (at least) and simply call grunt.file.readYAML().

Owner

cowboy commented Jul 30, 2014

@sindresorhus when you consider the effort involved in design, authoring and maintaining tooling, and supporting users, creating a new data format because the existing one isn't fast enough (I don't see how security is a concern with files you maintain) seems like a substantial amount of extra effort. Additionally, the solution I suggested has already been written, so it's zero extra effort.

So I don't see how it's easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment