dominictarr / rc

The non-configurable configuration loader for lazy people.
Other
1.02k stars 97 forks source link

Hjson Support #42

Closed laktak closed 9 years ago

laktak commented 9 years ago

I've added support for Hjson.

It's compatible with the current JSON + strip-comments but also supports a syntax specifically for config files that is less error prone. E.g. no trailing comma errors - for details see http://laktak.github.io/hjson/.

I'd appreciate if you could merge & publish to npm so we can use it with the slap editor.

dominictarr commented 9 years ago

I would love if it json didn't need commas, but I think this is just getting two far away from the standard. What is good about javascript is that it's ubiqutious, not that it's perfect. Comments is one thing, because it's still a subset of javascript, but with automatic commas means it's not javascript.

Though, I would certainly merge this if we got automatic commas added to js!

yuchi commented 9 years ago

<pedantic> JSON is not JavaScript </pedantic>

dominictarr commented 9 years ago

@yuchi I don't really think this is a fair argument - sure we are gonna use JSON parser to parse the config, not eval, but the important thing is that it should be a standard parser.

I think @ljharb is making a good argument here: https://github.com/dominictarr/rc/issues/38

rc aspires to simplicity and stability. I have always been very reluctant to change anything, and only do so after much discussion. I am willing to be persuaded - if you think it should be a certain way, you just have to make a good case for that.

ljharb commented 9 years ago

Honestly, I'd prefer an option to disable comments being allowed. I only want strict JSON in my modules' config files, which doesn't include comments, because that way every JSON tool works with it out of the box.

If this ever goes in, please make it opt-in, because I'd never want this option enabled in any of my projects (it'd be a dealbreaker for using rc for me)

dominictarr commented 9 years ago

@ljharb this particular issue is actually about comments, but about commas. we actually already have comments i'm afraid. that was merged some time ago.

ljharb commented 9 years ago

Yes, I'm aware :-) I was using that to illustrate that deviation from strict JSON is actually undesirable by default, so since we already have comments, we should not add any other deviations unless they're opt-in.

laktak commented 9 years ago

I agree that JavaScript needs commas and that JSON should not have comments.

JSON is a language-independent data interchange format. It's a great tool that does its job very well. Maybe too well. JSON is a great hammer but not everything is a nail.

Configuration files are edited by end-users, not developers. Users should not have to worry about putting commas in the correct place. Software should empower the user not hinder him.

Example: users often copy & paste configuration. When you do this with JSON the chance to make a mistake is very high.

IMO JSON is not the right tool for configuration files. rc already supports two formats although ini has its own problems. Hjson would not replace JSON.

If you want to make this opt-in we could add a #hjson header (currently it's considered ini unless the file starts with {).

dominictarr commented 9 years ago

@laktak another question: do you use ini? are you saying that slap considers it json if there is a {? I think that rc just tries to parse both ini and json and uses the first one that parses.

laktak commented 9 years ago

@dominictarr rc checks for .json or { (https://github.com/dominictarr/rc/blob/master/lib/utils.js#L8-15)

A JSON syntax error just throws.

slap just uses rc afaik.

howardroark commented 9 years ago

The reason I like the idea behind HJSON is that it offers a mental pivot point for people who are used to typing out JSON all the time. If you have ever written configs for Grunt.js in a team you will understand how something like a comma can really cause someone else grief.

I get that in the open-source world everyone spends ages looking things over and making sure it is prime time before they push. In a work setting when things just need to get out the door it is really great to have improvements like these in place.

My 2 cents is that if objects are being configured by humans to be used by machines then the machine can learn to do the hard work to cater for the human :) Plus more and more people out there are grokking the idea of configuring and even building things with objects.

I'm not really a programmer, just an advanced open-source user. There is a growing number of me out there though!

laktak commented 9 years ago

I've updated the PR. The Hjson format is now opt-in through the #hjson header.

ljharb commented 9 years ago

I'd like an option so that I, as the project maintainer, can explicitly forbid this format - I want to control the format of the config files my software supports, and not leave it up to the user. Can there also be an opt-in for rc itself?

laktak commented 9 years ago

@ljharb not sure why you are arguing against this.

rc loads and merges your config. Standard tools don't do that, you'd have to run it through rc to be sure about the config that is in use.

Wouldn't it be easier to save a running config (just stringify rc's result) that your users can send you for support?

ljharb commented 9 years ago

@laktak It increases the support burden for me, as a maintainer, for every additional config file format my users use. My ideal situation would be to only allow strict JSON, and nothing else, and to throw an error for the user when it finds an existing but invalid config file.

I have no objection to formats being available - only to them being forced upon me.

laktak commented 9 years ago

@dominictarr @ljharb I've changed the PR to:

The Hjson parser is not a dependency of rc. The dependencies section of your package.json file must contain the hjson module in order to read and parse Hjson configs.

So unless the app that is using rc also includes hjson in the package.json, nothing changes.

This is the same way https://github.com/lorenwest/node-config handles dependencies.

laktak commented 9 years ago

@dominictarr Would it be possible to merge this?

dominictarr commented 9 years ago

Okay, I have a compromise that doesn't complexify rc, but still gives everyone a way to do what they want. rc can add an option to take a parser function -

It should behave like JSON.parse - throwing if it's not valid, returning a js object if it is. @ljharb, you can pass in JSON.parse and he will not have ini or commented json. @laktak, you can pass in HTJSON.parse

by default, rc will use the ini or commented-json parser that it currently uses.

Okay, this is now implemented in rc@0.6, congratulations, I am very recluctant to add features to rc, but if you battle hard enough you may win.

laktak commented 9 years ago

works, thanks!