jslint

[kvz]

Hey guys,

I've done some improvements on the site. Some bugs have been fixed
(mostly comment&utf8 related) and I've made syntax highlighting look
the same throughout the site.
I've also added automatic jslint checking to every function page. You
can see it in action spotting errors here http://phpjs.org/functions/stripslashes:537
and here: http://phpjs.local/functions/get_html_translation_table:416

Right now it all happens clientside. We could change that later if
performance demands it.

Now would probably be a good time to decide what jslint options we
want to stick with. It's now configured as such:

var lintOptions = {
evil: true,
nomen: false,
onevar: false,
regexp: false,
strict: false
};

(except for strict, the options that Douglas Crockford uses on his own
fulljslint.js) but they may not suit our needs and iirc Brett had some
strong ideas on the topic.

Not jslint but related: I'd like to ask you to try to stick to a max
of 80chars per line and put comments on their own line where possible.
My IDE can have some more before it looks messy, but for the site &
other point of views it would look neat with 80 chars.

This is not a complaint about previous code, but I see we have some
inconsistencies here&there (we never had a policy about it). So if you
agree i'd like to add the convention to our coding standards so in the
future our code looks a bit more consistent.

[Brett Zamir]

On 11/30/2009 6:23 AM, kvz wrote:
> Hey guys,
>
> I've done some improvements on the site. Some bugs have been fixed
> (mostly comment&utf8 related) and I've made syntax highlighting look
> the same throughout the site.
>

Great. Is it possible to reenable the link to open a new page with the
code (so one can select all and copy without having to drag the mouse)?
I am very glad though that there is the option to copy without line
numbers appearing...

> I've also added automatic jslint checking to every function page. You
> can see it in action spotting errors here http://phpjs.org/functions/stripslashes:537
> and here: http://phpjs.local/functions/get_html_translation_table:416
>
>

Great!

> Right now it all happens clientside. We could change that later if
> performance demands it.
>
> Now would probably be a good time to decide what jslint options we
> want to stick with. It's now configured as such:
>
> var lintOptions = {
> evil: true,
> nomen: false,
> onevar: false,
> regexp: false,
> strict: false
> };
>
> (except for strict, the options that Douglas Crockford uses on his own
> fulljslint.js) but they may not suit our needs and iirc Brett had some
> strong ideas on the topic.
>
>

Yeah, I guess I do. :)

For one, I wish his "switch" dogma could be mitigated by an option to
allow fall-throughs (at least with a comment or something). It's almost
not worth using a switch at all if you don't allow fall-throughs.

I also didn't agree with not checking for CDATA closings.

I can't remember other gripes at the moment, though these gripes are of
course by someone who is very grateful for jslint.

As far as the specific options...

1) A pity, but in a few cases we do need "evil" (eval) to true. On the
other hand, maybe we should set it to false so A) We may notice other
functions which use it but shouldn't, and B) Others who visit the
website can see that it uses eval() ?

2) If we don't set "forin" to true, there will be a lot of work to do
(and I think there may be a few cases where we need to tolerate it).
However, it is not a bad idea to make our code work with other libraries
that override the prototype (e.g., someone had a problem using one of
our functions with Prototype.js).

3) Didn't you want "onevar" as true? Seems you are trying to only define
once at the top, no? Or is that because you need inner functions to each
have their own "var"?

4) I agree with "nomen" and "strict" to false, though I think those are
the defaults anyways (though it doesn't hurt to spell it out).

5) I agree with regexp being false (also the default), but I don't
understand the rationale of turning it on to true.

FYI, you may want to add the options also at namespaced_template.js (you
can overwrite the ones I had there, except for "global window".

As far as being even stricter:

1) As far as requiring capitalization of constructors, as checked in
"new" invocations (and not allowing invocations of upper-case functions
without "new"), I'd like to add that more stringent (and non-default)
requirement:
newcap: true

2) What do you think about strict white space (white), strict white
space indentation (indent), disallow undefined variables (undef),
disallow == and != (eqeqeq), require parens around immediate invocations
(immed)? These all seem they might be good to follow (even if it'd be a
lot of work to correct).

As far as the other more strict requirements, it seems we cannot use
Adsafe since it objects to "this", etc.: http://www.adsafe.org/ and I'm
not in favor of disallowing bitwise or ++/--.

> Not jslint but related: I'd like to ask you to try to stick to a max
> of 80chars per line and put comments on their own line where possible.
> My IDE can have some more before it looks messy, but for the site&
> other point of views it would look neat with 80 chars.
>
> This is not a complaint about previous code, but I see we have some
> inconsistencies here&there (we never had a policy about it). So if you
> agree i'd like to add the convention to our coding standards so in the
> future our code looks a bit more consistent.
>

A good idea, but unfortunately wrapping isn't a planned feature until at
least version 6.9 in Netbeans. However, I can try to remember to paste
into Notepad++ which at least shows the column. I read someone say
UltraEdit is the only one that auto-wraps at a given width.

best wishes,
Brett

[Kevin van Zonneveld]

On Mon, Nov 30, 2009 at 6:48 AM, Brett Zamir <bre...@gmail.com> wrote:
> Great. Is it possible to reenable the link to open a new page with the code
> (so one can select all and copy without having to drag the mouse)? I am very
> glad though that there is the option to copy without line numbers
> appearing...

I could probably add a copy-paste icon like github has
http://github.com/mojombo/clippy

In general, I'd rather have the rules perfect now (and maybe generate
tons of 'errors') than settle for lesser rules to avoid them. Even if
we may not be able to fix them immediately, they are good reminders
and maybe the site's commenters will even fix some of them. Seems as
an easy way to kill a lazy Sunday afternoon and score some medals : )
Also, good point about evil. Let's keep it.

Summarizing, how does this look to you?
http://wiki.github.com/kvz/phpjs/jslint-options

This list may contain jslint-defaults, but I basically want everything
in there that we have a clear opinion about. So if I missed something
feel free comment or change the document.

When we deem it awesome, I will modify the site & namespaced template
accordingly

>> Not jslint but related: I'd like to ask you to try to stick to a max
>> of 80chars per line
>

> A good idea, but unfortunately wrapping isn't a planned feature until at
> least version 6.9 in Netbeans. However, I can try to remember to paste into
> Notepad++ which at least shows the column. I read someone say UltraEdit is
> the only one that auto-wraps at a given width.

Well my point was I'd like us to write our code so that we don't need
wrapping : )
This makes it look good in every possible view / browser / program and
most importantly our site. Not having to scroll (or at least in only 1
dimension) and still being able to see everything is a treat : )
An additional benefit is that this rule can help with untangling code
responsibilities, which makes it easier to read & debug. Especially
when working with many people on the same code.

--
Met vriendelijke groet / Kind regards,

Kevin van Zonneveld
http://kevin.vanzonneveld.net

http://twitter.com/kvz

[Brett Zamir]

On 12/7/2009 6:09 AM, Kevin van Zonneveld wrote:
> On Mon, Nov 30, 2009 at 6:48 AM, Brett Zamir<bre...@gmail.com> wrote:
>
>> Great. Is it possible to reenable the link to open a new page with the code
>> (so one can select all and copy without having to drag the mouse)? I am very
>> glad though that there is the option to copy without line numbers
>> appearing...
>>
> I could probably add a copy-paste icon like github has
> http://github.com/mojombo/clippy
>
>

I'd hope we could avoid Flash if possible, though maybe that's the only
way (I know in Mozilla you need to enable an option in about:config to
do it from code)...

In any case, as it is now, when I highlight and copy the function, in a
number of cases, line breaks are messed up, so that really needs to be
addressed, I think...

Yeah, good idea...Will also help know whether we've already anticipated
policy on a new configuration option or not...

> So if I missed something
> feel free comment or change the document.
>
> When we deem it awesome, I will modify the site& namespaced template
> accordingly
>
>

Ok, I just went through to make sure we had everything from the options
table in http://www.jslint.com/lint.html . I didn't change any settings,
as they all looked ok to me. But maybe we should set evil to false so
that it will show up in case it gives anyone any ideas for a workaround
where we intended to use it, or warns us if we didn't mean to use it.
(Both cases are probably unlikely but maybe situations could change.).

Since you are defining the options as an object, we could also pass in
"predef" set to an array of predefined global variables (as strings). I
think we should start off conservative and not allow any (we don't even
need window except for the namespaced template itself I think), but we
can add any that come up.

>>> Not jslint but related: I'd like to ask you to try to stick to a max
>>> of 80chars per line
>>>
>> A good idea, but unfortunately wrapping isn't a planned feature until at
>> least version 6.9 in Netbeans. However, I can try to remember to paste into
>> Notepad++ which at least shows the column. I read someone say UltraEdit is
>> the only one that auto-wraps at a given width.
>>
> Well my point was I'd like us to write our code so that we don't need
> wrapping : )
> This makes it look good in every possible view / browser / program and
> most importantly our site. Not having to scroll (or at least in only 1
> dimension) and still being able to see everything is a treat : )
> An additional benefit is that this rule can help with untangling code

> responsibilities, which makes it easier to read& debug. Especially

> when working with many people on the same code.
>

If you can do it on the server-side, that'd be just great, but then we
wouldn't have to individually "stick to a max of 80 chars per line",
right? Or did you just mean we couldn't alter the ultimate output? That
would be fine...

[kvz]

On Dec 7, 5:44 am, Brett Zamir <bret...@gmail.com> wrote:
> In any case, as it is now, when I highlight and copy the function, in a
> number of cases, line breaks are messed up, so that really needs to be
> addressed, I think

Fair enough. The raw source link was from another plugin, but I added
a link to the raw github source, that should work just as well.

> > Summarizing, how does this look to you?
> >http://wiki.github.com/kvz/phpjs/jslint-options
>
> > This list may contain jslint-defaults, but I basically want everything
> > in there that we have a clear opinion about.
>
> Yeah, good idea...Will also help know whether we've already anticipated
> policy on a new configuration option or not...
>
> > So if I missed something
> > feel free comment or change the document.
>
> > When we deem it awesome, I will modify the site&  namespaced template
> > accordingly
>
> Ok, I just went through to make sure we had everything from the options

> table inhttp://www.jslint.com/lint.html. I didn't change any settings,

> as they all looked ok to me. But maybe we should set evil to false so
> that it will show up in case it gives anyone any ideas for a workaround
> where we intended to use it, or warns us if we didn't mean to use it.
> (Both cases are probably unlikely but maybe situations could change.).

Yes I agree. It must have thought evil: true would warn you about
evals, but apparently it's the other way around : )

> Since you are defining the options as an object, we could also pass in
> "predef" set to an array of predefined global variables (as strings). I
> think we should start off conservative and not allow any (we don't even
> need window except for the namespaced template itself I think), but we
> can add any that come up.

Ok let's do it on the wiki page so we're talking about the same thing.

> > Well my point was I'd like us to write our code so that we don't need
> > wrapping : )
> > This makes it look good in every possible view / browser / program and
> > most importantly our site. Not having to scroll (or at least in only 1
> > dimension) and still being able to see everything is a treat : )
> > An additional benefit is that this rule can help with untangling code
> > responsibilities, which makes it easier to read&  debug. Especially
> > when working with many people on the same code.
>
> If you can do it on the server-side, that'd be just great, but then we
> wouldn't have to individually "stick to a max of 80 chars per line",
> right? Or did you just mean we couldn't alter the ultimate output? That
> would be fine...

No I mean when we actually code, try to break the lines ourselves at
sensible places. Or try formulating the code so that it won't need >
80 chars per line. As a way of making our code look more consistent.

[Brett]

I've seen a number of complaints pile in to Netbeans recently to ask
for this feature to be moved up in priority. If that's in place, I
think I could more realistically manage that (I can try to estimate,
but not too easy).

By the way, with the "maxlen" property set to 80, lines like:

md5_file('http://kevin.vanzonneveld.net/
pj_test_supportfile_1.htm');

get complaints.

Also, the strict whitespace (white:true), my apologies, is completely
draconian. It won't allow function definitions like:

function is_int () {

because there is a space after is_int(). I've mentioned before that I
like the space there to be able to do finds which distinguish
invocations from definitions.

While I like spaces between mathematical operators, it also won't
allow something reasonable like:

arr[i+5] = 'a';

...insisting on spaces on the sides of the plus sign even in such a
case.

Maybe it's just me, but this seems pretty severe, without much benefit
(if you need to find something in code, regexp can easily skip over
inexact whitespace in most cases).

I'd guess Jslint will never be configurable to be tolerant of such
specific cases as above, but without such exceptions, imo, it's too
severe.

[Kevin van Zonneveld]

I think we should stick with the 80chars limit where reasonably
possible. Let jslint warn us about it to see if we can improve our
code and keep us sharp.

About whitespace: sure let's drop it then.

--

[Brett Zamir]

Ok.... There's been a lot of heated discussion of late encouraging
Netbeans to implement this, and it seems some work is underway, but it
is supposedly difficult for them to fix and may be a while. Oh
well...without the whitespace warnings, we'll be more able to notice the
80char warnings... :)

Brett

[Kevin van Zonneveld]

Cool, I set
http://wiki.github.com/kvz/phpjs/jslint-options

white: false

and also updated
/app/webroot/js/lint.js accordingly.

--