search

pattern-library / generator-pattern-library

Yeoman Generator creating Pattern Libraries
4 stars 4 forks source link

remove pattern-importer/code cleanup #1

Closed scottnath closed 9 years ago

scottnath commented 9 years ago

Hey @e2tha-e

This PR takes the changes from pattern-library-utilities and allows us to remove the pattern-importer repository all together. I also eslint-ed it and cleaned up the gulp file. Can you look this over please?

@rebmullin this also fixes your bug

scottnath commented 9 years ago

@e2tha-e

cc: @conortm @sergesemashko @beynya

anyone have time to look at this pull request today? pretty basic changes.

e2tha-e commented 9 years ago

@scottnath I'll take a loook at it now

e2tha-e commented 9 years ago

@scottnath This pr looks good. Before I merge, you might want to consider wrapping the entire js files in an IIFE and putting the 'use strict'; at the top of that, so linters don't complain and you don't have to repeat the 'use strict'; in each named function.

scottnath commented 9 years ago

hey @e2tha-e , node already does this. Adding an iife to node modules or a gulpfile is not necessary

https://nodejs.org/api/modules.html#modules_modules Variables local to the module will be private, as though the module was wrapped in a function. In this example the variable PI is private to circle.js.

as for 'use strict' and the gulpfile, it really seems like the best practice is to have just a single 'use strict' at the top of gulpfile.js only. See this example from the tests for gulp:

https://github.com/gulpjs/gulp/blob/master/test/watch.js

I've never seen a gulpfile with 'use strict' anywhere but there....

scottnath commented 9 years ago

or from some of their recipes:

https://github.com/gulpjs/gulp/blob/506bac3f9f243cfda402b56143e2de2bae9418bc/docs/recipes/handling-the-delete-event-on-watch.md

e2tha-e commented 9 years ago

@scottnath Yes. I wasn't recommending this for any purpose other than to abide by linters that work across browser js and node js. If you're ok with leaving this pr as is, I'll merge it

scottnath commented 9 years ago

@e2tha-e totally ok with as is

(perhaps) unfortunately, node js and browser js are different beasts and will most likely require a different set of linting requirements.

e2tha-e commented 9 years ago

@scottnath If we decide to go that route, I have no objection, and would be willing to use commonly used and peer-reviewed node js standards for node files. Merging

scottnath commented 9 years ago

@e2tha-e thanks