Improve css method

[jugglinmike]

Bring Cheerio's css method closer to jQuery's. Specifically:

• Implement new signatures
  • Setting via a function
  • Getting via an array
• Implement API for unsetting styles (via the empty string)
• Fix a bug when setting multiple elements' styles at once
• Remove unspecified 0-argument signature

Additionally, document the method in the project's Readme.md file

[jugglinmike]

@matthewmueller or @davidchambers Would you mind giving this a review?

[davidchambers]

I wonder whether get and set should be proper methods (possibly underscore-prefixed). It's unusual to have a function which can only be profitable invoked via Function::call or Function::apply with a suitable context argument.

If these functions are to remain detached from Cheerio.prototype, I suggest we continue to take the cheerio object as an argument.

[davidchambers]

Be careful:

> typeof null
"object"
> typeof new Number(42)
"object"

[jugglinmike]

I could use _.isObject to protect against the null case, but I'm not sure how to guard against new Number(42). We could check prop.__proto__ to see if it is a "plain" object, but that seems overly prescriptive. In realitive, I think code like $("div").css(new Number(42)) is pretty uncommon, and even if it were used, the behavior is more-or-less expected: the own properties of the object (of which there would be none) are copied into the selection's style attribute.

So what do you think of simply:

 exports.css = function(prop, val) {
   if (arguments.length === 2 ||
-    (typeof prop === 'object' && !_.isArray(prop))) {
+    (_.isObject(prop) && !_.isArray(prop))) {

[davidchambers]

I'd consider this the canonical check:

Object.prototype.toString.call(prop) === '[object Object]'

[jugglinmike]

Hmm, okay. I'll note that Lodash would give us the more readable/concise _.isPlainObject, which is another (albeit small) case for #182.

[davidchambers]

Though I really appreciate having access to Underscore (or Lo-Dash) in this project, I believe @matthewmueller is keen to remove the dependency at some point. It's probably best to stick to the Object.prototype.toString check for now, in any case.

[fb55]

@davidchambers He also said he's planning a rewrite, so for the current code base, it should be safe to use a _ library.

[matthewmueller]

yah it's fine for now. if/when i get around to a re-write it will be under a different name. will work in the browsers too so underscore / lo-dash would add unnecessary bulk.

[jugglinmike]

I wonder whether get and set should be proper methods (possibly underscore-prefixed). It's unusual to have a function which can only be profitable invoked via Function::call or Function::apply with a suitable context argument.

If these functions are to remain detached from Cheerio.prototype, I suggest we continue to take the cheerio object as an argument.

Personally, I think writing "private" functions with a semantic self keyword (versus a custom self argument) makes reading those functions easier, and that this benefit outweighs the annoyance of call invocation.

That said, this is definitely a style issue (and one that can be generalized for the whole project). I'm happy to go either way so long as we're consistent. If @matthewmueller can break the tie, then I will update the pull request (as necessary), and include the consensus in our CONTRIBUTING.md file.

[davidchambers]

Personally, I think writing "private" functions with a semantic self keyword (versus a custom self argument) makes reading those functions easier, and that this benefit outweighs the annoyance of call invocation.

[…]

I'm happy to go either way so long as we're consistent.

I don't have a strong preference for a particular approach, though I'm pleased to have raised the issue for discussion.

There are actually three options to consider:

1. Private function which takes a gizmo as its first argument:

var foo = function(gizmo, ...) { ... };

// usage
foo(gizmo, ...);

2. Private function which must be invoked on a gizmo:

var foo = function(...) { ... };

// usage
foo.call(gizmo, ...);

3. "Private" method:

Gizmo.prototype._foo = function(...) { ... };

// usage
gizmo._foo(...);

My objection to the second option is that such functions do not make their requirements clear to the reader. As a reader, I don't expect to need to look for occurrences of this within a function to determine whether it takes an additional, "undocumented" argument.

[davidchambers]

Aside from the above, which requires discussion, this pull request looks good to me. Nice work, as always, @jugglinmike. :)

[matthewmueller]

my personal use of a private method is either:

a) treat it no differently than a prototypal method, just add a comment in the code, but don't document it in the readme.
b) if it's a small utility, unrelated to the prototype and doesn't need any context, put it at the bottom as function blah() { ... }.

[matthewmueller]

this looks good @jugglinmike. thanks man