dominictarr / rc

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

parser signature is not respected #58

Closed MoOx closed 9 years ago

MoOx commented 9 years ago

In utils.js, the signature of parse() is (content, file). From what I understand of the body of the parser, file should be the filename.

But from what I see in the module, I don't find a place where file is actually provided

Is it a mistake ?

MoOx commented 9 years ago

That might be useful for custom parser in order to throw an exception with the reference to the file that is causing the issue (using content in an exception might now help).

dominictarr commented 9 years ago

ah, there is a comment explaining why the filename is passed in: https://github.com/dominictarr/rc/blob/df21a2637a2ff80abd6380f178f765c81c5c2684/lib/utils.js#L8-L11

which obviously got overridden at some point, except that since it also checked for { it is still working. I think it would be a better move to remove the file argument all together.

instead of having the parser take the filename and through a custom error, it would be better for it to just throw a normal error, and that then be caught by rc, which could then add a message about the particular file that was invalid. (if you really needed that)

MoOx commented 9 years ago

I don't really need that, but that might help end user. I am actually throwing an exception https://github.com/MoOx/rc-loader/blob/3d4e05bbf1a3b0d3f646349efb8f39a/src/parser.js#L15-L18 so I guess you could enhance this exception from this module directly. No need for the filename for me.

dominictarr commented 9 years ago

hmm, why are you using stripJsonComments for if it uses yaml?

MoOx commented 9 years ago

yaml is a superset of json, so this parsing include json parsing. I have taken this line from eslint https://github.com/eslint/eslint/blob/95214607920931af83af19bf8ae8da761fa49b13/lib/config.js#L74

dominictarr commented 9 years ago

wow that is complicated!

dominictarr commented 9 years ago

the file argument has been officially removed in v 1.1.1

MoOx commented 9 years ago

That's kind of a breaking change... Not cool to deliver as a patch :(

dominictarr commented 9 years ago

no this is not a breaking change, because it was never called, therefore nothing could have been depending on it, so can't have broken anything.

dominictarr commented 9 years ago

I only removed dead code - the parser had an file argument, but it never got passed anything - so even if you used a custom parser, it wouldn't have been passed file either, so this change wouldn't have broken you either.

MoOx commented 9 years ago

ok sorry for the noize.

dominictarr commented 9 years ago

no problem!