Use JSCS preset system instead of generating custom build config. #70

rwjblue · 2015-10-14T17:10:40Z

This PR removes the custom config file generation and replaces it with JSCS's built in preset system (thanks to @hzoo for helping push that forward so much!). This removes the need for a custom binary/command (because now jscs command will work properly).

I believe this is backwards compatible (if folks had created a .jscsrc file with our rules in them, they should continue to work just the same). Definitely open to thoughts of how that might be an issue (worst case scenario we can just bump majors, but I'd prefer not to)...

Fixes #64.
Fixes #34.

rwjblue · 2015-10-14T17:12:08Z

tests/fixtures/.jscsrc

@@ -0,0 +1 @@
+{ "preset": "../../lib/jscsrc.json"}


This lets JSCS find lib/jscsrc.json (which is exactly what a user would get if they did preset: 'ember-suave').

rwjblue · 2015-10-14T17:14:45Z

Note: that I used ember-cli-blueprint-test-helpers to make testing the generated file simple. Thanks to @trabus for his hard work on extracting those helpers.

elidupuis · 2015-10-14T17:54:18Z

Just tested this out quickly for my integration with pre-commit and it seems to work great! 👍

rwjblue · 2015-10-14T18:16:07Z

aptible/ember-json-schema-document#1 updates a little addon that was previously using a custom .jscsrc (for editor integrations) and all seems to work well.

hzoo · 2015-10-14T18:16:10Z

maybe add preset or jscs-preset to keywords of package.json?

Btw we're working on 3.0 soon (going to release a bugfix release today) mainly for https://github.com/cst/cst but also other stuff like "verbose": true by default

rwjblue · 2015-10-14T18:18:05Z

@hzoo - Great idea, updated!

hzoo · 2015-10-14T18:28:08Z

How different is ember-suave and the ember .jscsrc anyway? You could potentially use ember-suave as a preset although you would have to manage the versions (well I guess that its true already)

rwjblue · 2015-10-14T18:31:57Z

ember-suave is quite a bit more aggressive than Ember's default JSCS setup (forbidding var, requiring destructuring, etc).

brzpegasus · 2015-10-15T02:11:34Z

@rwjblue This looks great!! Much cleaner with a preset! Just a few things:

Would you mind squashing the commits?
I'm happy to merge this. However, I will want to update the README to reflect the changes before releasing.
Does this classify as backwards-compatible if the user has to do more than simply upgrade ember-suave in order for it to work? Mainly:
- They will need to run ember g ember-suave at the very least, after bumping the version (or after automatically getting the updated version when they run npm install if they specified ember-suave with ~ or ^).
- If the project previously had a custom .jscsrc (which ember-suave was using to merge with the default rules), it would need to be moved out of the way in favor of the .jscsrc file from the blueprint, and then the contents from the old file would need to be merged back in. Otherwise, things will not run out of the box.

brzpegasus · 2015-10-15T02:18:12Z

If the project previously had a custom .jscsrc, [...] things will not run out of the box.

Here's an example of a custom .jscsrc file that a project may have created, without including all the rules from ember-suave. In other words, the file could have been created only to override some of the rules from ember-suave, but does not otherwise list out all of the rules:

{
  "excludeFiles": ["fixtures/**"],
  "requireCamelCaseOrUpperCaseIdentifiers": "ignoreProperties",
  "requireCommentsToIncludeAccess": null
}

This would stop working after upgrading, without some (minimal) changes.

rwjblue · 2015-10-15T12:25:41Z

Would you mind squashing the commits?

No problem. Done.

I will want to update the README to reflect the changes before releasing.

Agreed. Will get that updated shortly.

They will need to run ember g ember-suave at the very least, after bumping the version (or after automatically getting the updated version when they run npm install if they specified ember-suave with ~ or ^).

It really depends on what we want. It is possible for us to detect that no .jscsrc file is present and print a console warning and provide a custom one for them (essentially bring back the jscsrcBuilder in the case that no .jscsrc file is found).

If the project previously had a custom .jscsrc (which ember-suave was using to merge with the default rules), it would need to be moved out of the way in favor of the .jscsrc file from the blueprint, and then the contents from the old file would need to be merged back in. Otherwise, things will not run out of the box.

Same as above, we could bring back the jscsrcBuilder and have it detect if "preset": "ember-suave" is present in .jscsrc. If it is not present, we would do the current merging behavior with a console warning. If it is present, we do nothing and use their .jscsrc.

It all comes down to what we want. Do we want to have this addon have a nice upgrade path (following Ember's concept of "no major version breakage without prior deprecation") or do we want to make this addon as simple as possible and revv the major version?

brzpegasus · 2015-10-15T13:56:22Z

README.md

+}
+```
+
+`ember-sauve` integrates well with ember-cli, but can also be used as a standalone JSCS preset. This allows custom


s/sauve/suave/

Good catch! Fixed...

rwjblue · 2015-10-15T13:58:02Z

It all comes down to what we want. Do we want to have this addon have a nice upgrade path (following Ember's concept of "no major version breakage without prior deprecation") or do we want to make this addon as simple as possible and revv the major version?

Damn that stupid Ember project setting good examples.

I updated this to provide deprecation warnings when used in the following scenarios:

without a project .jscsrc file
with a project .jscsrc file that does not contain "preset": "ember-suave"

All existing scenarios should fall into one of those two deprecations and the current setup is still supported. I think this is not fully backwards compatible. When we decide to bump major versions (likely for JSCS 3.0) we can remove lib/jscsrc-builder.js and tests/jscsrc-builder-test.js.

brzpegasus · 2015-10-15T14:04:22Z

Damn that stupid Ember project setting good examples.

😛 Was hard arguing against not being a good citizen and providing an upgrade path.

I think this is not fully backwards compatible.

You mean "now fully backwards compatible", right?

brzpegasus · 2015-10-15T14:05:18Z

lib/jscsrc-builder.js

  }
 }

+function getSuaveConfig() {
+  // avoiding `require` (which would have worked fine)
+  // to prevent mutating the cached result


rwjblue · 2015-10-15T14:05:57Z

You mean "now fully backwards compatible", right?

LOL, yes

rwjblue · 2015-10-15T14:30:49Z

Updated to prevent printing deprecation message twice (once for app tree and once for tests tree). The deprecation messages are now only logged once.

Also, add blueprint and blueprint test. Now: ``` ember install ember-suave ``` Will auto-generate a `.jscsrc` with `{ "preset": "ember-suave" }`.

brzpegasus · 2015-10-15T14:38:22Z

Excellent, thanks!! 💯

Use JSCS preset system instead of generating custom build config.

rwjblue reviewed Oct 14, 2015
View reviewed changes

rwjblue mentioned this pull request Oct 14, 2015

Use JSCS preset version of ember-suave. aptible/ember-json-schema-document#1

Merged

rwjblue force-pushed the jscs-presets branch from 29b6200 to 296db62 Compare October 14, 2015 18:17

rwjblue force-pushed the jscs-presets branch from 0668c42 to e2be779 Compare October 15, 2015 00:01

rwjblue force-pushed the jscs-presets branch 3 times, most recently from 9128ed2 to bcb22ca Compare October 15, 2015 13:55

brzpegasus reviewed Oct 15, 2015
View reviewed changes

rwjblue force-pushed the jscs-presets branch from bcb22ca to 4ee6a97 Compare October 15, 2015 13:58

brzpegasus reviewed Oct 15, 2015
View reviewed changes

rwjblue force-pushed the jscs-presets branch 2 times, most recently from 448f844 to 87cb441 Compare October 15, 2015 14:30

Allow usage as JSCS preset.

717db06

Also, add blueprint and blueprint test. Now: ``` ember install ember-suave ``` Will auto-generate a `.jscsrc` with `{ "preset": "ember-suave" }`.

rwjblue force-pushed the jscs-presets branch from 87cb441 to 717db06 Compare October 15, 2015 14:34

brzpegasus added a commit that referenced this pull request Oct 15, 2015

Merge pull request #70 from rwjblue/jscs-presets

6a48c95

Use JSCS preset system instead of generating custom build config.

brzpegasus merged commit 6a48c95 into DavyJonesLocker:master Oct 15, 2015

rwjblue deleted the jscs-presets branch October 15, 2015 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use JSCS preset system instead of generating custom build config. #70

Use JSCS preset system instead of generating custom build config. #70

rwjblue commented Oct 14, 2015

rwjblue Oct 14, 2015

rwjblue commented Oct 14, 2015

elidupuis commented Oct 14, 2015

rwjblue commented Oct 14, 2015

hzoo commented Oct 14, 2015

rwjblue commented Oct 14, 2015

hzoo commented Oct 14, 2015

rwjblue commented Oct 14, 2015

brzpegasus commented Oct 15, 2015

brzpegasus commented Oct 15, 2015

rwjblue commented Oct 15, 2015

brzpegasus Oct 15, 2015

rwjblue Oct 15, 2015

rwjblue commented Oct 15, 2015

brzpegasus commented Oct 15, 2015

brzpegasus Oct 15, 2015

rwjblue commented Oct 15, 2015

rwjblue commented Oct 15, 2015

brzpegasus commented Oct 15, 2015

Use JSCS preset system instead of generating custom build config. #70

Use JSCS preset system instead of generating custom build config. #70

Conversation

rwjblue commented Oct 14, 2015

rwjblue Oct 14, 2015

Choose a reason for hiding this comment

rwjblue commented Oct 14, 2015

elidupuis commented Oct 14, 2015

rwjblue commented Oct 14, 2015

hzoo commented Oct 14, 2015

rwjblue commented Oct 14, 2015

hzoo commented Oct 14, 2015

rwjblue commented Oct 14, 2015

brzpegasus commented Oct 15, 2015

brzpegasus commented Oct 15, 2015

rwjblue commented Oct 15, 2015

brzpegasus Oct 15, 2015

Choose a reason for hiding this comment

rwjblue Oct 15, 2015

Choose a reason for hiding this comment

rwjblue commented Oct 15, 2015

brzpegasus commented Oct 15, 2015

brzpegasus Oct 15, 2015

Choose a reason for hiding this comment

rwjblue commented Oct 15, 2015

rwjblue commented Oct 15, 2015

brzpegasus commented Oct 15, 2015