jonschlinkert / grunt-readme

DEPRECATED. Use Verb instead
https://github.com/assemble/verb
Other
30 stars 10 forks source link

treat variables as immutable #45

Closed dylang closed 10 years ago

dylang commented 10 years ago

This is something I find useful that I learned from my coworker @nickheiner that I want to pass along.

Treat all variables as immutable

I found code to be difficult to follow because values like options.templates are changed to functions.

 if(_.isEmpty(options.templates)) {
      options.templates = root('templates');
    } else {
      options.templates = bind(options.templates);
    }
    var templates = options.templates;

Here's some suggestions:

var templates = _.isEmpty(options.templates) 
 ? root('templates')
 : bind(options.templates);

or

// more descriptive than "bind" and "root"
function asPathResolver(filepath, defaultFilepath) {
  if (_.isEmpty(filepath)) {
    return _.partial(path.join, __dirname, '../', defaultFilepath);
  }
  return _.partial(path.join, filepath, '');
}
var templates = asPathResolver(options.templates, 'templates');

Note - I think creating a partial is what you were doing with bind, but I didn't test that to see if it would work, baby is crying gotta go!

dylang commented 10 years ago

Here's another example:

// Extract content from template
    options.readme = yaml.extract(options.readme).content;

I had options.readme set to a value, but I was getting an error that suggested I wasn't setting a value. I didn't realize the value had to be a yaml file with a specific key.

dylang commented 10 years ago

Oops, in my exhaustion I didn't realize what the problem was. It's that no matter what options.readme is overwritten even if you provide a valid alternative filepath.

// If `readme` option is defined with a readme template, use that,
    // otherwise, see if the README template is in the `docs()` dir
    if (!options.readme && grunt.file.exists(docs('README.tmpl.md'))) {
      options.readme = docs('README.tmpl.md');
    // As a last resort, use a template from grunt-readme
    } else {
      options.readme = templates('README.tmpl.md');
    }
jonschlinkert commented 10 years ago

Yes! this is one of the reasons i decided to refactor. I was hoping no one noticed lol

jonschlinkert commented 10 years ago

..and yes, bind is similar to using a lodash _.partial.

jonschlinkert commented 10 years ago

@dylang I'm sorry that it took so long to address these issues! I just wanted to let you know that we are deprecating grunt-readme, since I just launched a new and improved version of this: Verb.

I also released grunt and gulp tasks, a cli and a generator (the grunt task can do multiple targets, or one, or none!) . Verb does what grunt-readme does, but super easy to use (requiring zero config) and can be extended with plugins, custom tags, etc. It's still very new, but I value your feedback so let me know if you have any suggestions. (btw, I use your projects all the time, thanks for all the OS contributions!)

dylang commented 10 years ago

That's great, looking forward to checking out Verb!

I've been using grunt-readme with great success, you can see the similar templates:

All of them are handled by this repo which also owns the reusable templates like the header, footer, and badges:

jonschlinkert commented 10 years ago

Awesome! that's very cool. we need to exchange ideas sometime for sure! you have some great projects.

jonschlinkert commented 10 years ago

btw, this is how I'm getting around having to define a target in the gruntfile - for when the defaults are enough: https://github.com/assemble/grunt-verb/blob/master/tasks/verb.js#L50-L68.