New config loading rules

[gustavohenke]

By default, when the config option is undefined, or when it's true, the JSCS config loader will be used with no specific file passed in.
If config is a string, it will use the config loader only to parse it, as it used to be until now
If a falsy value is given, no config loading will be done and the config will be empty.

Also, this PR fixes some "conditional" unit tests.

Fixes #20

[gustavohenke]

@markelog and @mikesherov, any opinions?
If we're good to move on, I'll publish another minor version soon.

[Krinkle]

Looks good (though the pr changes a lot of tests, not sure if that was intentional).

I'd recommend maybe not exposing the undefined value as an API and instead use grunt's default options feature to have true be the actual default. That way if somebody passes undefined for whatever weird reason, it'll simply be casted to boolean false, and it'd simplify the documentation as well.

In the tasks/jscs.js file you'd use something like:

- options = this.options(),
+ options = this.options(
+     config: true
+ } ),

[gustavohenke]

Good catch @Krinkle!

The tests were intentionaly changed because they were "conditional" (inside an catch block only, for example -- what should happen if it didn't throw?), which would not work with the new functionality.

[Krinkle]

@gustavohenke I don't really know anything about the internals of this grunt task. I just found the changes seemingly unrelated to changing the default value of the config option. There doesn't appear to be any exception throwing going on inside tasks/lib/jscs.js. There could be exceptions thrown indirectly via the calls to grunt.*, but this commit doesn't appear to change tasks/lib/jscs.js in a way that would explain why the tests need such major refactoring (it looks like it would only have to change one or two test cases to no longer expect an error or to expect an error where we previously didn't).

Anyway, I guess you're just taking the opportunity to clean up other things at the same time. No need to explain it to me :-)

[markelog]

Sorry, i didn't chime in at the moment, was busy working on releasing jscs.

I'd recommend maybe not exposing the undefined value as an API and instead use grunt's default options feature to have true be the actual default

Agree on undefined, but not so sure about default true value since purpose of the #20 is to not implicitly merge the inline options with .jscsrc options if config attribute was not specified.

I like the idea of merging inline options and .jscsrc options, but do that explicitly. But we could just not merge them together, so if user had specified inline options and config file - use only options of config file, if there is only options without config prop use only inline options. I don't like this approach through, but maybe it's just me?

Originally, it was my intention to expose jscs logic for config search through true value and
throw a warning if no config nor inline options were specified, with default null value.

[mikesherov]

While I agree mostly @markelog, I think parity with JSHint is the easiest for devs to understand.

All I know is that you should not have to explicitly specify the config file in the absence of rules defined in the config.

The following is what I expect the behavior to be:

1. Specify both config file and rules: merge.
2. Specify just rules: just use rules, don't load config.
3. Specify just config: just use that config
4. Specify neither: use .jscsrc
5. Specify "true" as config: use .jscsrc

Lastly, we can add a new config option: jscsrc: true which is clearest of all.

[gustavohenke]

throw a warning if no config nor inline options were specified, with default null value.

I think this is a good idea.

Specify just rules: just use rules, don't load config.

This one makes sense, should be done.

Specify neither: use .jscsrc
Specify "true" as config: use .jscsrc

Or .jscs.json or package.json, as defined in JSCS config loader, you mean @mikesherov?

[mikesherov]

Or .jscs.json or package.json, as defined in JSCS config loader, you mean @mikesherov?

correct.

[markelog]

@mikesherov sounds much more reasonable :-)

Specify neither: use .jscsrc
Specify "true" as config: use .jscsrc

So true value by default and no warning message?

Lastly, we can add a new config option: jscsrc: true which is clearest of all.

Which would be just an alias for config option? Parity with grunt-contrib-jshint and all that?

[mikesherov]

Which would be just an alias for config option? Parity with grunt-contrib-jshint and all that?

Basically, except the only possible value is true, or don't specify it.

[mikesherov]

So true value by default and no warning message?
correct.

[markelog]

That's two very similar options, i would go with one: config, with true and String types, since if we will have two, it would be confusing: since grunt-contrib-jshint could have String but we can't and what shall we do in case they both specified.

It might easier to think that grunt-contrib-jshint jshintrc option is the same as ours config prop.

[mikesherov]

That's two very similar options, i would go with one: config, with true and String types, since if we will have two, it would be confusing: since grunt-contrib-jshint could have String but we can't and what shall we do in case they both specified.

OK. Fair enough.

[Krinkle]

+1. Having two options seems confusing. Having a "jscsrc" would match grunt-contrib-jshint a bit, but if we would implement it as only taking boolean we might as well not try to be like grunt-contrib-jshint since it'd be quite different still. In fact, I'd argue keeping just config and have it accept boolean or string is more like grunt-contrib-jshint (just a different name).

[gustavohenke]

Do you think we could use grunt.warn instead for these messages, @markelog?

[markelog]

Yeah, sounds more user friendly, although its weird that user would want force execution in this situation

[gustavohenke]

OK, will do after this PR gets merged in

[gustavohenke]

@mikesherov, @markelog, @Krinkle, anyone has further doubts about this PR after the last commits?

[mikesherov]

undefined is not a string or a boolean.

[gustavohenke]

Will adjust the docs!

[gustavohenke]

I've updated the docs to match the new behavior of the config option.
Are we good to merge now?

[mikesherov]

Lgtm!

[Krinkle]

The documented behaviour looks good (I haven't verified the implementation). I'm not quite sure what the purpose is of having both null and true though. If we use true as the actual default value the end result would be the same and we can get rid of null all-together and simplify handling to only boolean/string. Right?

[gustavohenke]

null/undefined would be the unspecified config.

If you specify true, .jscsrc will be loaded if it exists.