New format attempt

[markelog]

WIP

Related to #136
This is what i got so far - https://gist.github.com/markelog/752e53faf9f27de4024d

/cc @mikesherov @mdevils @mrjoelkemp @nschonni

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[mikesherov]

A couple of questions we must answer:

1. How does a user nullify rules?
2. How can we make sure this config lends itself to automatic configuration?

[markelog]

How does a user nullify rules?

curlyBraces: false or

curlyBraces: {
require: false,
disallow: false
}

How can we make sure this config lends itself to automatic configuration?

What ticket number are we talking about?

[hzoo]

is it #516? Auto formatting API

[markelog]

Is

Auto formatting

the same as

automatic configuration

#341 ?

[hzoo]

Ok good point - I don't think so.

Auto formatting seems to mean it will actually correct the code style errors of rules already specified/known. Automatic configuration is changing the jscsrc file to match a specified file or determine code style rules automatically?

[markelog]

Correct

[mikesherov]

Yes, #341.

[mrjoelkemp]

Nice work, Oleg.

How are you going to map the configuration items to the existing rules? The current way is nice because we rely on the user to indicate the rules to use. Is the plan to change the returned value from each rule's getOptionName to indicate the category (ex: "blocks") and the rule ("curlyBraces")?

A few nitpicks:

https://gist.github.com/markelog/752e53faf9f27de4024d#file-gistfile1-js-L15

except instead of expect

https://gist.github.com/markelog/752e53faf9f27de4024d#file-gistfile1-js-L45

To keep with the style, maybe empty instead of emptyBlocks?

[qfox]

Is the plan to change the returned value from each rule's getOptionName to indicate the category (ex: "blocks") and the rule ("curlyBraces")?

Btw It would be great to use in plugins.

[mrjoelkemp]

@zxqfox How would that help plugins? Can you elaborate?

[mikesherov]

More questions:

1. How does the new format handle "require spaces around only + but disallow spaces around all except - ?"
2. How does the new format handle "jQuery preset but disallow spaces inside parens instead of require"?
3. How does the new format handle validateIndentation?
4. How does the new format specify validateParameterSeparator, or any rule where "around" isn't adequate for spacing?

[mikesherov]

Just a thought... Instead of

{
require: true
disallow: true
only: []
except: []
}

Which doesn't seem to allow for "require spaces around only + but disallow spaces around all except - ", we do:

{
require: true || { except|only: [] },
disallow: true || { except|only: [] }
}

Which would cover "require spaces around only + but disallow spaces around all except - " as follows:

{
require: { only: ['+'] },
disallow: { except: ['-'] }
}

[markelog]

@mrjoelkemp

Is the plan to change the returned value from each rule's getOptionName to indicate the category (ex: "blocks") and the rule ("curlyBraces")?

Sounds reasonable.

A few nitpicks:

Fixed

@mikesherov

How can we make sure this config lends itself to automatic configuration?

We don't have module for it yet, i suppose we could only guess, but i don't see problems with this ticket with any config style we choose.

How does the new format handle "require spaces around only + but disallow spaces around all except - ?"

Your idea looks better:

{
    require: true || { except|only: [] },
    disallow: true || { except|only: [] }
}

How does the new format handle validateIndentation?

Didn't get to that part yet, requires more thought i guess. One way or another it would be sane to have no more than two levels of nesting objects, like this:

"function": {
    "spacesInFunction": {
        "anonymousExpression": {
            ...
        }
        ...
    },

    "validate": {
        "argumentSeparator": ...
    }
}

Would be overdoing it i guess.

[qfox]

require|disallow: true || { except|only: [] } looks nice. But how to map this configuration declarations to final rules?

In common:

{
  "category": {
    "rule": {
      "require": true || { "allExcept|only": [] },
      "disallow": true || { "allExcept|only": [] }
    }
  }
}

But once again, I'm stuck in mapping these declarations to rules.

[markelog]

But once again, I'm stuck in mapping these declarations to rules.

Let's decide on format first, there is a lot of grey areas we need to cover first.

[qfox]

@mrjoelkemp Some thoughts about plugins.

1. At this moment there is no API for plugins except additionalRules. And it loads your rules and passes only parts of configuration to configure method;
2. Each rule now have concrete definition in configuration (configuration-rules mapping). And it means two different rules can't use the same data from configuration (as far as I know);
3. We have plugin API in development and it would be great if @mdevils says something about his thoughts about it.

Finally I have a lot of my own thoughts (partially realized in jscs-jsdoc) but each of them very variative.

So I think configuration format topic is very close to plugins because of configuration-rules mapping.

p.s. I ❤️ new format at all.

[qfox]

One way or another it would be sane to have no more than two levels of nesting objects, like this:

@markelog It's like you giving scissors to kids. Rules and plugins shouldn't be developed by someone who doesn't know how to do it right. This nesting could be very useful for plugins, but I agree that 3-rd level of nesting should be used very-very rarely or never.

[markelog]

to all: there is a lot of rules to adopt for this config, we also need identify rules that we would move to the plugin i.e. rules that could not be used in any of supported presets and those rules that could not be easily included in any category (first level).

So if you have time, please go ahead and add rest of the rules to the new format, i will update the gist with our new ideas shortly.

[mdevils]

We have plugin API in development and it would be great if @mdevils says something about his thoughts about it.

We discussed this approach with Oleg earlier and I like it.

@mikesherov,

require: true || { except|only: [] }, — I like this!

[markelog]

Format updated, stopped at disallowDanglingUnderscores rule

[nschonni]

Sorry for the late reply.
This appears to be the format from #181, but as pointed out there it doesn't prevent conflicting rules like the format in #182 does.

[markelog]

This appears to be the format from #181

Yeah, maybe In some ways but i'm trying to take best ideas from all purposed formats.

but as pointed out there it doesn't prevent conflicting rules like the format in #182 does.

It could be handled on engine level, if, for example, there both require and disallow properties are defined - we could throw an error.

If you have time to add more rules to the gist above or provide more constructive critic - that would be much appreciated.

[mikesherov]

but as pointed out there it doesn't prevent conflicting rules like the format in #182 does.

Keep in mind conflicting rules aren't always conflicting. I could require space between + and disallow -.

Although, if there is a rule that should be only require XOR disallow, we could enforce that at engine level, as @markelog described.

[nschonni]

Sorry if this is coming off as non-constructive, but I was just trying to flag that the nature of the format has some issues as raised in #698 (comment)
I do like the idea of supporting a filter function for the rules that currently have array values.

[mdevils]

just like right know or module name with a mandatory prefix jscs-

Bad idea. It's better to go nodejs-way: interpret preset name as a path if it starts with ./, .. or /.

Presets and additional rules (I believe) perfectly works when provided by plugins. I would not like to allow specifying preset as an NPM-module name. That's plugins are made for.

[markelog]

Plugins could express both preset and additionalRules, so why would we need those options?

[markelog]

Couple new rules, fixes, comments, stopped at requireAlignedObjectValues.

[qfox]

I propose to have some config proxy because of a big lag between this research and v2.0.

What if we will try to load configuration with a new way and convert to current naming if it has options property or some cli flag? It would allow us to make an old names→new names map, and we will never forget a rule. While browserified version can be left as is before v2.0.

[mrjoelkemp]

@zxqfox Sounds interesting, but can you supply an example of the options property and the cli flag? I'm not seeing the full picture.

[qfox]

Oh. ;-) I'm talking about @markelog's gist: https://gist.github.com/markelog/752e53faf9f27de4024d#file-gistfile1-js-L7

The current .jscsrc format doesn't have this property while the new one will have it. So if this property exists in config then we should use proxy. And when we will go to v2 we'll just drop this condition (or will rewrite it somehow to detect and notify, or even autoconvert an old format to the new one).

And the cli option can be simply --try-v2-config to force using configuration proxy.

p.s. I'm also thinking about different inline disables. This converter also could help users to convert v1 disables to v2.

[qfox]

@markelog https://gist.github.com/markelog/752e53faf9f27de4024d#file-gistfile1-js-L89 btw. array keyword in a singular while all others in plural. It's ok?

[Krinkle]

@markelog

How does a user nullify rules?
curlyBraces: false or [..]

Hm.. Maybe keep using null for that. In my opinion it's not clear whether false means that the rule is disabled or that the rule is in "disallow" mode.

We could also support boolean values for the common/simple use case of only requiring and disallowing rules.

E.g. true as short cut for { require: true } and (maybe) false as shortcut for { disallow: true }.

{
  "blocks": {
    "curlyBraces": true,
    "newlineBefore": false,
    "paddingNewlines": null
  }
}

Though it may not be obvious whether false means it doesn't care (disabled rule), or that it is enabled and actively disallows use of the pattern. Another way would be to use strings instead of booleans. It'd be more verbose but re-uses the same words for clarity:

{
  "blocks": {
    "curlyBraces": "require",
    "newlineBefore": "disallow",
    "paddingNewlines": null
  }
}

[qfox]

@Krinkle Yeah, I've thought about it too. Like these booleans usage. It's pretty intuitive.

[hzoo]

For new/modified rules now, is it still

 * "disallowDanglingUnderscores": { allExcept: ["_exception"] }

because some rules like requireSpacesInsideParentheses are using

 * "requireSpacesInsideParentheses": {
 *     "all": true,
 *     "except": [ "{", "}" ]
 * }

[mikesherov]

Just wanted to drop a note that the "2-level" approach to configuration here means:

1. the way plugins and getOptionName work needs to change.
2. the way enable/disable comments works needs to change.

I'm not sure again as to the benefit of having 2 levels for rules. It introduces the question of what belongs in the top level vs. the bottom level.

If the options are all normalized, simplified (meaning combining beforeAndAfter and require/disallow) and some names shortened, 1 level looks fine too IMO:

"binarySpacing": "require",
"unarySpacing": { require: { allExcept: "++" } },
"curlyBraceBlocks": "require",
"curlyBraceNewlines": "require"

Just something to consider.

[markelog]

I think we're at the point of prototyping this thing, i'm sure will find more caveats along the way or possible improvements.

[mrjoelkemp]

A bit of discussion on this issue going on in Gitter: https://gitter.im/jscs-dev/node-jscs?at=55566279811d64626eebcb40.

We're torn between forcing users to change their configuration in 2.0 versus having an internal conversion proxy to allow users to not have to change their config.

[markelog]

For this issue to resolve i think we need to:

• declaratively specify rule options Declaratively specify rule option values #480, Add ability to declare acceptable values for the rules #895
• preserve back-compat - both old and new format should work
• create classification strategy of which rules should go to which block, like "block", "vars", etc; see the proposed format
• actually add new parsing for the new format
• improve documentation, since new format would be defined by the "blocks" we need to clearly document who to do that
• rules clean-up, at least Standardise terminology around line-break, line-feed or newline #853, requireSpacesInsideObjectBrackets using inconsistent terminology #538 disallowMultipleVarDecl: cleanup #1349

Only last item is a breaking change.

[markelog]

In other words, is there any takers for the proposed items?

[mrjoelkemp]

Yes, I'm looking at #895.