Configuration: design review #672

mdevils · 2014-10-07T20:02:04Z

Plugin support with a bunch of configuration simplification.

I suggest to simplify configuration routines to a pair of files:

Configuration — base configuration class. Works in any environment, used by StringChecker.
NodeConfiguration (temporal name) — node.js-specific configuration class. Works in node.js.

This replaces all configuration code in Checker, StringChecker, files at lib/options. Can replace routines at CLI-module during further development.

Still we can keep full backward compatibility. This is just an internal redesign.

/cc @markelog, @mikesherov, @mrjoelkemp

mikesherov · 2014-10-08T12:07:52Z

lib/config/configuration.js

+ * Registers built-in Code Style cheking rules.
+ */
+Configuration.prototype.registerDefaultRules = function() {
+    this.registerRule(new (require('./../rules/require-curly-braces'))());


can you leave a comment here explaining why we need to explicitly enumerate each rule. I know it's for browserify, but would be good to have that noted in the code.

Yep, was thinking about it, thank you!

mikesherov · 2014-10-08T12:09:01Z

Would love to see how this gets used in Checker and StringChecker. Seeing an implementation would help me review. Looks great so far.

mrjoelkemp · 2014-10-09T03:58:31Z

I like it too. Having this configuration logic in a single place makes the flow clearer. +1 for the plugin hooks. Great job.

Closes gh-677 Fixes #665

mdevils · 2014-10-13T23:44:25Z

Integrated into StringChecker/Checker, going to publish tomorrow, some fixes are still required. No compatibility broken yet.

Closes gh-560 Closes gh-680 Fixes #561

mdevils · 2014-10-14T20:33:21Z

Published version with StringChecker / Checker integration. Check it out please. If you find it OK, I will remake CLI configuration (going to simplify it using the same classes).

mikesherov · 2014-10-14T20:37:04Z

lib/checker.js

+    this._fileExtensions = this._configuration.getFileExtensions();
+
+    this._excludes = this._configuration.getExcludedFiles().map(function(pattern) {
+        return new minimatch.Minimatch(path.resolve(cwd, pattern), {


I believe this should exist inside the configuration module

In max-errors.js, Object.defineProperties cast the config maxErrors property to Number but not the StringChecker maxErrors property. This leads to a bug if multiple jscs checks are executed using the same config object (config.maxerrors will equal 'undefined' the first time, and 'NaN' the second time)

Closes gh-688 Closes gh-687

When a switch case only uses returns instead of breaks, treat the return indentation as the break indentation. Closes gh-686 Fixes #684

These options were surrounded by single quotes instead of backticks, so they were not formatted as code like the other options. Closes gh-683

Fixes #693

Closes gh-692 Fixes #691

coveralls · 2014-10-26T18:03:28Z

Coverage decreased (-0.48%) when pulling 0d6d62f on mdevils.plugins into c70fec7 on mdevils.camel-case.

When there are multiple spaces between parameters, throw an error until it matches the specified seperator value. Fixes #701 Closes #707

Documentation, spaces, code style issues and add tests for "true" value Closes #586

mdevils · 2014-10-28T02:00:59Z

What I have done:

Finished Configuration tests.

Next steps:

Write NodeConfiguration tests.
Rebase and implement esnext and esprima options.
Port the error about non-camelcase options.

coveralls · 2014-10-28T02:06:01Z

Coverage decreased (-0.27%) when pulling 2e9fafa on mdevils.plugins into c70fec7 on mdevils.camel-case.

mdevils · 2014-10-28T19:00:28Z

What I have done:

Finished NodeConfiguration tests.

Next steps:

Rebase and implement esnext and esprima options.
Port the error about non-camelcase options.

I'm almost there! 😄

coveralls · 2014-10-28T19:11:18Z

Coverage increased (+0.09%) when pulling 7891e18 on mdevils.plugins into c70fec7 on mdevils.camel-case.

Closes gh-725 Fixes #723

Closes #724

mdevils · 2014-10-30T00:21:21Z

What I have done:

Ported the error about non-camelcase options.

Next step:

Rebase and implement esnext and esprima options.

coveralls · 2014-10-30T00:23:45Z

Coverage increased (+0.14%) when pulling ea29ea5 on mdevils.plugins into c70fec7 on mdevils.camel-case.

… code.

mdevils · 2014-10-30T22:11:07Z

Finished my work. Check it out: #731

coveralls · 2016-04-12T08:55:13Z

Coverage decreased (-5.4%) to 86.943% when pulling 5e8e4c8 on mdevils.plugins into c70fec7 on mdevils.camel-case.

mdevils force-pushed the mdevils.plugins branch 3 times, most recently from ccad22d to c7682bb Compare October 7, 2014 20:12

mikesherov reviewed Oct 8, 2014
View reviewed changes

Docs: add a table of contents to README

d42dbab

Closes gh-677 Fixes #665

mdevils force-pushed the mdevils.camel-case branch from 13d35db to c70fec7 Compare October 13, 2014 00:15

rwjblue and others added 7 commits October 14, 2014 08:46

Parsing: Allow both harmony and custom esprima versions.

013cd7e

Closes gh-560 Closes gh-680 Fixes #561

StringChecker: update docblock for constructor

f4b3a5d

Prepare for version 1.7.0

1c39a12

1.7.0

f7eb084

validateIndentation: fix empty multiline function body regression

39053f7

Prepare for version 1.7.1

e83b235

1.7.1

c80f5f2

mdevils force-pushed the mdevils.plugins branch from c7682bb to 3567027 Compare October 14, 2014 20:31

mikesherov reviewed Oct 14, 2014
View reviewed changes

mdevils force-pushed the mdevils.plugins branch 2 times, most recently from 4bf82c0 to 0f2a630 Compare October 14, 2014 20:42

Florian Fesseler and others added 9 commits October 15, 2014 10:42

StringChecker: unit test for when maxErrors is NaN

d4c7b25

Closes gh-688 Closes gh-687

validateIndentation: fix return in switch failure

032b937

When a switch case only uses returns instead of breaks, treat the return indentation as the break indentation. Closes gh-686 Fixes #684

Fix format of --esnext and --max-errors in README

9cf0ffe

These options were surrounded by single quotes instead of backticks, so they were not formatted as code like the other options. Closes gh-683

CLI: fix order of options in help screen

4cca407

Prepare for version 1.7.2

2d80942

1.7.2

dfd3020

validateIndentation: handle breakless case stmts

0979aaf

Fixes #693

Parsing: Use the harmony parser via the esnext flag in the config

c311492

Closes gh-692 Fixes #691

hzoo and others added 5 commits October 26, 2014 23:29

validateParameterSeparator: fix for multiple spaces between parameters

c891fbf

When there are multiple spaces between parameters, throw an error until it matches the specified seperator value. Fixes #701 Closes #707

New Rules: (require | disallow)PaddingNewlinesBeforeKeywords

79dc5d3

*PaddingNewlinesBeforeKeywords: fix minor issues

44d24b5

Documentation, spaces, code style issues and add tests for "true" value Closes #586

Docs: Correct docs for requireAnonymousFunctions rule

7795fff

Update dependencies

d2206fd

mdevils force-pushed the mdevils.plugins branch from 02239af to 2e9fafa Compare October 28, 2014 02:03

hzoo added 3 commits October 28, 2014 21:24

disallowDanglingUnderscores: Support an array of additional exceptions

086031e

Closes gh-725 Fixes #723

Docs: make indentation to be consistent at 4 spaces

e30c58a

Docs: add more specific cases for function spaces rules

101eccb

Closes #724

mdevils force-pushed the mdevils.plugins branch from 0e876f6 to ea29ea5 Compare October 30, 2014 00:20

mdevils added 9 commits October 30, 2014 20:28

Remove underscore option name error reporting

2e79b6a

Configuration: design review

7ca8217

Some Configuration/NodeConfiguration tests

8a8a9b8

More tests for Configuration. Included config tests. Updated some CLI…

7de5241

… code.

Configuration: tests, overrides

74e4863

Configuration tests

1e8be34

Full Configuration/NodeConfiguration test coverage

87191b0

Throwing an error in case of underscore-type config

60587f7

Rebase and implement esnext option

5e8e4c8

mdevils force-pushed the mdevils.plugins branch from ea29ea5 to 5e8e4c8 Compare October 30, 2014 20:21

mdevils closed this Oct 30, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration: design review #672

Configuration: design review #672

mdevils commented Oct 7, 2014

mikesherov Oct 8, 2014

mdevils Oct 8, 2014

mikesherov commented Oct 8, 2014

mrjoelkemp commented Oct 9, 2014

mdevils commented Oct 13, 2014

mdevils commented Oct 14, 2014

mikesherov Oct 14, 2014

mdevils Oct 15, 2014

coveralls commented Oct 26, 2014

mdevils commented Oct 28, 2014

coveralls commented Oct 28, 2014

mdevils commented Oct 28, 2014

coveralls commented Oct 28, 2014

mdevils commented Oct 30, 2014

coveralls commented Oct 30, 2014

mdevils commented Oct 30, 2014

coveralls commented Apr 12, 2016

Configuration: design review #672

Configuration: design review #672

Conversation

mdevils commented Oct 7, 2014

mikesherov Oct 8, 2014

Choose a reason for hiding this comment

mdevils Oct 8, 2014

Choose a reason for hiding this comment

mikesherov commented Oct 8, 2014

mrjoelkemp commented Oct 9, 2014

mdevils commented Oct 13, 2014

mdevils commented Oct 14, 2014

mikesherov Oct 14, 2014

Choose a reason for hiding this comment

mdevils Oct 15, 2014

Choose a reason for hiding this comment

coveralls commented Oct 26, 2014

mdevils commented Oct 28, 2014

coveralls commented Oct 28, 2014

mdevils commented Oct 28, 2014

coveralls commented Oct 28, 2014

mdevils commented Oct 30, 2014

coveralls commented Oct 30, 2014

mdevils commented Oct 30, 2014

coveralls commented Apr 12, 2016