Skip to content

Commit

Permalink
whitespace, trailing space, and doc fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
notslang committed Apr 14, 2014
1 parent bb4c51e commit 6544413
Show file tree
Hide file tree
Showing 27 changed files with 348 additions and 424 deletions.
1 change: 0 additions & 1 deletion bin/roots
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,3 @@ if (!args.quiet) {

// pass the arguments to the cli module
cli.execute(args, notifier.packageFile)

2 changes: 1 addition & 1 deletion docs/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@

# Add any paths that contain templates here, relative to this directory.
templates_path = ['_templates']

# The suffix of source filenames.
source_suffix = '.rst'

Expand Down
2 changes: 1 addition & 1 deletion docs/configuration.rst
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ An array containing `minimatch <https://github.com/isaacs/minimatch>`_ strings t

**dump_dirs**

Array of directories that will have their contents dumped into the output folder rather than compiling into the folder they are in.
Array of directories that will have their contents dumped into the output folder rather than compiling into the folder they are in.
*default: ``['views', 'assets']``*

**env**
Expand Down
2 changes: 1 addition & 1 deletion docs/environments.rst
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ To compile with an environment, you can pass an ``--env`` or ``-e`` flag to the
If you are using the javascript api, you can pass an ``env`` option to the ``Roots`` constructor as such:

.. code-block:: javascript
var Roots = require('roots');
var project = new Roots(__dirname, { env: 'production' })
Expand Down
6 changes: 3 additions & 3 deletions docs/extensions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ There is one more hook you can use that will fire only when all the files in a g
module.exports = ->
class FooBar
category_hooks: ->
after: (ctx, category) ->
console.log "finished up with #{category}!"
Expand Down Expand Up @@ -263,7 +263,7 @@ For example, if you were making an extension that collected all the contents of
nodefn = require('when/node/function')
path = require('path')
fs = require('fs')
module.exports (opts) = ->
class JSConcat
Expand Down Expand Up @@ -296,7 +296,7 @@ What we have here is a simple extension that concatenates js files into a single
nodefn = require('when/node/function')
path = require('path')
fs = require('fs')
module.exports (opts) = ->
contents = ''
Expand Down
99 changes: 43 additions & 56 deletions lib/api/compile.coffee
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
fs = require 'fs'
W = require 'when'
nodefn = require 'when/node'
guard = require 'when/guard'
keys = require 'when/keys'
fs = require 'fs'
W = require 'when'
nodefn = require 'when/node'
guard = require 'when/guard'
keys = require 'when/keys'

This comment has been minimized.

Copy link
@jescalan

jescalan Apr 14, 2014

I know that you do not, but I like having my variables aligned up top, so I'd like to keep this style

This comment has been minimized.

Copy link
@notslang

notslang Apr 14, 2014

Author Collaborator

ugh... but then we lose compliance with polarmobile's coffeescript styleguide :P

This comment has been minimized.

Copy link
@jescalan

jescalan Apr 14, 2014

Sorry! As much as I probably love polar mobile, what I love more is code that is as readable as possible

This comment has been minimized.

Copy link
@jescalan

jescalan Apr 14, 2014

And like I have mentioned before, while I do of course appreciate that you are taking some much time to look through the style for every project, I would rather spend time working on important features than arguing about small code style things.

This comment has been minimized.

Copy link
@notslang

notslang Apr 14, 2014

Author Collaborator

"probably love polar mobile"? lol, it's the style guide linked to in the contributing file. I assumed that you read through that.

As for spending time looking through the style of projects - I generally just reformat as I read. But I've stopped submitting those during the week.

And the whole point of these PR's is to improve readability - I think that following the standard that polarmobile made is a pretty good way to do that.

This comment has been minimized.

Copy link
@notslang

notslang Apr 14, 2014

Author Collaborator

Oh, and if you have a good explanation for why/how to align variable declarations, then feel free to jump on the discussion here: polarmobile/coffeescript-style-guide#2 (or migrate that part of the discussion to a new issue)

sequence = require 'when/sequence'
mkdirp = require 'mkdirp'
mkdirp = require 'mkdirp'

FSParser = require '../fs_parser'
Compiler = require '../compiler'
Expand All @@ -18,23 +18,20 @@ class Compile

###*
* Creates a new instance of the compile class.
*
* - makes a new fs parser instance
* - makes a new compiler instance
* - makes a new instance of each extension, with error detection.
* this must happen every compile pass to clear lingering context
*
* @param {Function} roots - instance of the base roots class
* @param {Function} roots - instance of the base roots class
###

constructor: (@roots) ->
@extensions = @roots.extensions.instantiate()
@fs_parser = new FSParser(@roots, @extensions)
@compiler = new Compiler(@roots, @extensions)

###*
* Compiles the project. This process includes the following steps:
*
* - execute user before hooks if present
* - parse the project, sort files into categories
* - create the folder structure
Expand All @@ -59,7 +56,6 @@ class Compile

###*
* Calls any user-provided before hooks with the roots context.
*
* @private
###

Expand All @@ -68,7 +64,6 @@ class Compile

###*
* Calls any user-provided after hooks with the roots context.
*
* @private
###

Expand All @@ -77,12 +72,10 @@ class Compile

###*
* Checks to ensure the requested hook(s) is/are present, then calls them,
* whether there was an array of hooks provided or just a single hook.
*
* @private
*
* @param {Array|Function} hook - a function or array of functions
whether there was an array of hooks provided or just a single hook.
* @param {Array|Function} hook - a function or array of functions
* @return {Promise} promise for resolved hooks
* @private
###

hook_method = (hook) ->
Expand All @@ -99,8 +92,7 @@ class Compile

###*
* If present, runs an async-compatible `setup` function in each extension,
* ensuring that any asynchrnonous setup the extension needs is completed.
*
ensuring that any asynchrnonous setup the extension needs is completed.
* @return {Promise} a promise that the extension setup is finished
###

Expand All @@ -109,11 +101,10 @@ class Compile
W.map(req_setup, ((ext) -> ext.setup()))

###*
* Creates the nested folder structure for a project. First, creates an array
* of just the output paths, then creates the base public folder, then
* sequentially walks through the folders and creates them all.
*
* @param {Object} ast - roots ast
* Creates the nested folder structure for a project. First, creates an
array of just the output paths, then creates the base public folder, then
sequentially walks through the folders and creates them all.
* @param {Object} ast - roots ast
###

create_folders = (ast) ->
Expand All @@ -125,28 +116,26 @@ class Compile

###*
* Files are processed by category, and each category can be processed in
* One of two ways: parallel or ordered. Parallel processed categories will
* crunch through their files as quickly as possible, starting immediately.
* Ordered categories will parallel compile all the files in the category, but
* wait until one category is finished before moving to the next one.
*
* An example use for each of these is client templates and dynamic content.
* With client templates, they do not depend on any other compile process so
* they are a great fit for parallel. For dynamic content, the front matter must
* be parsed then available in normal templates, which means all dynamic content
* must be finished parsing before normal content starts. For this reason, dynamic
* content has to be ordered so it is placed before the normal compiles.
*
* So what this function does is first distinguishes ordered or parallel for each
* extension, then pushes a compile task for that extension onto the appropriate
* stack. The compile task just grabs the files from the category and runs them
* each through the compiler's `compile` method. Then when they are finished, it
* runs the after category hook.
*
* Once the ordered and parallel stacks are full of tasks, they are run. Ordered
* gets sequenced so they run in order, and parallel runs (surprise) in parallel.
*
* @param {Object} ast - roots ast
One of two ways: parallel or ordered. Parallel processed categories will
crunch through their files as quickly as possible, starting immediately.
Ordered categories will parallel compile all the files in the category,
but wait until one category is finished before moving to the next one. An
example use for each of these is client templates and dynamic content.
With client templates, they do not depend on any other compile process so
they are a great fit for parallel. For dynamic content, the front matter
must be parsed then available in normal templates, which means all
dynamic content must be finished parsing before normal content starts.
* For this reason, dynamic content has to be ordered so it is placed before
the normal compiles. So what this function does is first distinguishes
ordered or parallel for each extension, then pushes a compile task for
that extension onto the appropriate stack. The compile task just grabs
the files from the category and runs them each through the compiler's
`compile` method. Then when they are finished, it runs the after category
hook.
* Once the ordered and parallel stacks are full of tasks, they are run.
Ordered gets sequenced so they run in order, and parallel runs (surprise)
in parallel.
* @param {Object} ast - roots ast
###

process_files = (ast) ->
Expand Down Expand Up @@ -179,16 +168,14 @@ class Compile
parallel: W.all(parallel)

###*
* Sometimes extensions prevent file writes and leave behind empty
* folders. The client templates extension is a good example. No matter
* how it happens, there should not be any empty folders in the output,
* so this method gets rid of them if they exist.
*
* Sometimes extensions prevent file writes and leave behind empty folders.
The client templates extension is a good example. No matter how it
happens, there should not be any empty folders in the output, so this
method gets rid of them if they exist.
* The way this is done is *very* hacky, but it is the speediest way. It
* tries to delete every folder, and if it succeeds, it means the folder was
* empty, as trying to remove a directory with contents throws an error (which
* we ignore using an empty callback).
*
tries to delete every folder, and if it succeeds, it means the folder was
empty, as trying to remove a directory with contents throws an error
(which we ignore using an empty callback).
* @private
###

Expand Down
55 changes: 27 additions & 28 deletions lib/api/new.coffee
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
path = require 'path'
fs = require 'fs'
path = require 'path'
fs = require 'fs'
{EventEmitter} = require('events')
exec = require('child_process').exec
nodefn = require 'when/node'
sprout = require 'sprout'
global_config = require '../global_config'
_ = require 'lodash'
npm = require 'npm'
exec = require('child_process').exec
nodefn = require 'when/node'
sprout = require 'sprout'
global_config = require '../global_config'
_ = require 'lodash'
npm = require 'npm'

###*
* @class New
Expand All @@ -19,23 +19,25 @@ class New extends EventEmitter
@base_url = 'https://github.com/roots-dev/base.git'

###*
* Main method, given a path to where the project should be and some (optional)
* additional options, creates a new project template. If no template is provided,
* uses the roots default template, which is installed if not present. Once the
* template is created, installs dependencies if a package.json is present.
*
* @param {Object} opts - Arguments object, takes the following:
* - path: path to nonexistant folder where project should be
* - template: name of the template to use for the project
* - options: overrides for template config
* - defaults: default values for template config
* Main method, given a path to where the project should be and some
(optional) additional options, creates a new project template. If no
template is provided, uses the roots default template, which is installed
if not present. Once the template is created, installs dependencies if a
package.json is present.
* @param {Object} opts - Arguments object, takes the following:
* @param {string} opts.path - path to nonexistant folder where project
should be
* @param {string} opts.template - name of the template to use for the
project
* @param {Object} opts.options - overrides for template config
* @param {Object} opts.defaults - default values for template config
###

exec: (opts) ->
@path = opts.path || throw new Error('missing path')
@template = opts.template || global_config().get('default_template')
@overrides = opts.options || {}
@defaults = opts.defaults || {}
@path = opts.path || throw new Error('missing path')
@template = opts.template || global_config().get('default_template')
@overrides = opts.options || {}
@defaults = opts.defaults || {}

@pkg = path.join(@path, 'package.json')
@defaults.name = opts.name
Expand All @@ -52,8 +54,7 @@ class New extends EventEmitter

###*
* Uses sprout.init to create a project template, emits events, and installs
* dependencies if necessary.
*
dependencies if necessary.
* @private
###

Expand All @@ -68,9 +69,8 @@ class New extends EventEmitter
.done((=> @emit('done', @path)), ((err) => @emit('error', err)))

###*
* Tests whether a project has a package.json file and therefore needs to have
* dependencies installed.
*
* Tests whether a project has a package.json file and therefore needs to
have dependencies installed.
* @private
* @return {Boolean} whether a package.json file exists in the template
###
Expand All @@ -80,7 +80,6 @@ class New extends EventEmitter

###*
* Uses npm to install a project's dependencies.
*
* @private
* @return {Promise} a promise for installed deps
###
Expand Down
8 changes: 4 additions & 4 deletions lib/api/template.coffee
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
sprout = require 'sprout'
_ = require 'lodash'
W = require 'when'
sprout = require 'sprout'
_ = require 'lodash'
W = require 'when'
global_config = require '../global_config'
fs = require 'fs'
fs = require 'fs'

exports.add = sprout.add.bind(sprout) # TODO: prepend all templates with "roots-"
exports.remove = sprout.remove
Expand Down
18 changes: 8 additions & 10 deletions lib/api/watch.coffee
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
chokidar = require 'chokidar'
chokidar = require 'chokidar'
minimatch = require 'minimatch'
_ = require 'lodash'
_ = require 'lodash'

###*
* @class Watcher
Expand All @@ -13,8 +13,8 @@ class Watcher

###*
* Compile the project, once done, watch it for further changes.
*
* @return {Object} chokidar [https://github.com/paulmillr/chokidar] instance
* @return {Object} chokidar [https://github.com/paulmillr/chokidar]
instance
###

exec: ->
Expand All @@ -30,13 +30,11 @@ class Watcher
return _.extend(@roots, { watcher: watcher })

###*
* Given a path, returns true or false depending on whether it should be ignored
* or not.
*
* @private
*
* @param {String} p - absolute file path
* Given a path, returns true or false depending on whether it should be
ignored or not.
* @param {String} p - absolute file path
* @return {Boolean} whether the file should be ignored or not
* @private
###

ignore = (p) ->
Expand Down
2 changes: 1 addition & 1 deletion lib/cli/clean.coffee
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
path = require 'path'
path = require 'path'
Roots = require '../'
rimraf = require 'rimraf'

Expand Down
2 changes: 1 addition & 1 deletion lib/cli/compile.coffee
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
path = require('path')
path = require('path')
Roots = require('../')

module.exports = (args, cli)->
Expand Down
Loading

8 comments on commit 6544413

@samccone
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SO, I find these commits happening across multiple projects as more and more code gets written. These "whitespace" changes are hard to always catch in code review thus these massive whitespace cleanup PR's.

If you are going to accept this @Jenius I would say that it needs to be paired with a test to prevent these errors in the future.

See this PR for how to
marionettejs/backbone.marionette@ce62531

It then puts the responsibility on the contributor to use proper white-space.

@notslang
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for that. most of it should be enforced by the .editorconfig, but it's a great idea to have travis monitor it.

@samccone
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is only enforced by .editorconfig if they have it enabled and that is a big if

@notslang
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, exactly why travis is a good idea for this

@jescalan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like, looking through this discussion you linked @slang800, both the author of the guide and jashkenas agreed that an uninterrupted series of variable declarations do usually look better aligned correctly. This is the exact way that I treat the declarations here -- the only ones that get aligned are the variables in a big uninterrupted block at the top of files.

polarmobile/coffeescript-style-guide#2 (comment)

Again, I prefer doing it this way, your link shows that I am clearly not the only one, so unless you have some really major objections let's keep it this way.

As for the whitespace thing, I think maybe having it run coffeelint before the tests might be a good idea. I'll tinker around with a setup like that this week sometime!

@notslang
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, nobody questions that aligned variables look good (even I think they look pretty), but it's balancing that decorative formatting with:

  • increased maintenance
  • difficulty of dealing with edge cases (like when one variable is way longer than the others)
  • the way it screws up diffs when you need to add or remove whitespace from like 5 lines just because the longest variable in that block was removed or renamed.
  • defining rules to describe when to align, and when it goes too far (both styleguide rules and linter rules)

This is why it hasn't been added to the styleguide yet - it makes formatting a lot more complicated...

IMO, we should develop a way for editors to apply styles like aligning assignments as a view that is independent of the code. That way the code could be treated as data, while the editor displays it according to the user's preferences... might have to look at that once I start using atom.

@jescalan
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@notslang
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean a package that edits the underlying code (I've seen plenty of those) - I mean one that changes the way that you see the code (adjusting it to whatever your preferences are), while leaving the actual source the same.

Please sign in to comment.