Merge lp://staging/~danilo/launchpad/bug-772754-other-subscribers-sections into lp://staging/launchpad

Hi Danilo,

I really like your approach to JS development and enjoy learning from your branches.

Things like the CSS_CLASSES dict are a nice touch to encapsulate info in a DRY fashion.

* Y.Node.create('<div></div>') can be written as Y.Node.create('<div />') and many find it more readable. YUI does the right thing, so you can even use it on elements that aren't really allowed to be self-closing.

* Defining 'level' at first use, maybe in getOrCreateSection, would be helpful.

I see you use a lot of bare comparisons against null and undefined whereas others seem to prefer to use the Y.Lang tests. Are they equivalent? Perhaps this is an issue we should decide on and put in our (as yet mythical) JS Guidelines. Your thoughts are appreciated.

Thanks for catching the feature flag that I think should've already been removed.

Thank Brad (both for kind words and a review)!

У пет, 10. 06 2011. у 18:09 +0000, Brad Crittenden пише:

> * Y.Node.create('<div></div>') can be written as Y.Node.create('<div />') and many find it more readable. YUI does the right thing, so you can even use it on elements that aren't really allowed to be self-closing.

Oh, I was not aware that YUI was smart enough: I specifically trained
myself to use the more verbose variant so I don't mess it up when it
won't work. I'll fix all the use in my code now.

> * Defining 'level' at first use, maybe in getOrCreateSection, would be helpful.

I assume you mean a more detailed description of the parameter? I've
added that, and referred the reader to `subscriber_levels` as defined
earlier in the JS file, and BugNotificationLevel enum in Python from
there.

> I see you use a lot of bare comparisons against null and undefined
> whereas others seem to prefer to use the Y.Lang tests. Are they
> equivalent? Perhaps this is an issue we should decide on and put in
> our (as yet mythical) JS Guidelines. Your thoughts are appreciated.

I mix them myself. Whenever I find it useful, I use Y.Lang.is* tests,
but sometimes I just naturally go for "=== null", especially with node
comparisons (as all YUI3 node methods return null when no node is
matching). Also, I see a lot of value in Y.Lang.isValue (which checks
for both null and undefined) and similar methods, but Y.Lang.isNull not
so much: it's just a more expensive "=== null" check. I personally,
don't feel we should aim for consistency regarding something this simple
especially at the cost of performance (no matter how negligible the hit
might be).

I still haven't changed this, but if you feel strongly about it, I'd be
happy to accommodate :)

(I won't be landing this until all the branches in sequence are ready to
land)

> Thanks for catching the feature flag that I think should've already been removed.

The feature flags are not yet removed, but all the code that Gary and I
did in these branches is not conditional on them, so it's moot to use
them. We decided to remove the feature flags once these branches land,
considering it's easier to re-do the feature flag removal than these.

Incremental diff attached if you are interested.

Thanks again,
Danilo