mhorvath2161 wrote:

unified diff file

Attached:

mhorvath2161 wrote:

unified diff file

Update. Forgot some stuff.

Attached:

mhorvath2161 wrote:

The changes don't include the other proposed change: remove all semicolons.

I never use semicolons in javascript. They're not necessary unless you put multiple statements on the same line, and I think they're ugly. More dead trees.

mhorvath2161 wrote:

unified diff file

Update. More stuff. The patch is cumulative.

Attached:

It looks like all you've done here is take wikibits.js and change it to use your own personal style preference.

The {}'s inside of code are perfectly valid inside of code even when on single statements. They aid readability of the blocks.
In fact the way you remove some {}'s and put 'else' statements on the start of the line makes some statements harder to read rather than easier.

You've moved a few multiline statements onto single lines. This has made some of them longer, normally it's a good idea to split long statements rather than merge them. It makes them more readable because the longer lines trail past the edge of some editors.

And I STRONGLY object to the proposal of removing semicolons. They are NOT useless. Lint validation of JS will even spit out a pile of warnings if you give it code without ;s. The ;s help to avoid anything broken. And it's perfectly possible for people to want to minify the code as well to combine it with other files, in fact Wikia does minify wikibits.js and combine it with YUI and an number of other JS libraries. Minification requires ;s to be used, without it you end up breaking the code.

And don't give me the "This style of code reduces the byte size of the file reducing the data transfered to the browser" crap. That's complete and utter nonsense.
Code is meant to be readable, not small. If you want small then you minify code. Using a minifier would reduce the code size far more than your style changes, and wouldn't make things less readable.

Additionally, I brought up the point of minification in the past. It's pointless. Gzipping makes minification useless, just gzipping the file reduces it's size by an insane amount, so much so that even if you minify it after that there is almost no difference in the file size.

This patch merely changes wikibits.js to a single user's style preferences and makes code less readable. It has no benifit to the code. Recommend WONTFIX.

mhorvath2161 wrote:

(In reply to comment #5)

It looks like all you've done here is take wikibits.js and change it to use
your own personal style preference.

First of all, I simply changed the code to consistently use a single style of all the styles already used in the file. I didn't add any styles of my own.

The {}'s inside of code are perfectly valid inside of code even when on single
statements. They aid readability of the blocks.
In fact the way you remove some {}'s and put 'else' statements on the start of
the line makes some statements harder to read rather than easier.

I didn't add any statements where 'if' or 'else' appear at the beginning of a line followed by a statement. Those already existed in the code; I simply left them alone. I don't find an absence of brackets around single lines of code to be any less readable. This is already done in several places in the code; I just changed it so the same style is used consistently.

You've moved a few multiline statements onto single lines. This has made some
of them longer, normally it's a good idea to split long statements rather than
merge them. It makes them more readable because the longer lines trail past the
edge of some editors.

I don't find splitting individual statements across multiple lines to be easier to read. It's confusing and makes it look like there are really multiple statements, with each level of nesting resulting in additional indentation.

And I STRONGLY object to the proposal of removing semicolons. They are NOT
useless. Lint validation of JS will even spit out a pile of warnings if you
give it code without ;s. The ;s help to avoid anything broken. And it's
perfectly possible for people to want to minify the code as well to combine it
with other files, in fact Wikia does minify wikibits.js and combine it with YUI
and an number of other JS libraries. Minification requires ;s to be used,
without it you end up breaking the code.

Fair enough. If semicolons are useful for minifation, then that's a good reason not to remove them. However, I just downloaded the version of wikibits from wn-Wikipedia, and it doesn't look minified to me.

http://en.wikipedia.org/skins-1.5/common/wikibits.js?169

Also, there's no presence of any YUI code as far as I can see.

And don't give me the "This style of code reduces the byte size of the file
reducing the data transfered to the browser" crap. That's complete and utter
nonsense.

Thanks for remaining civil. It shows great consideration on your part.

Code is meant to be readable, not small. If you want small then you minify
code. Using a minifier would reduce the code size far more than your style
changes, and wouldn't make things less readable.
Additionally, I brought up the point of minification in the past. It's
pointless. Gzipping makes minification useless, just gzipping the file reduces
it's size by an insane amount, so much so that even if you minify it after that
there is almost no difference in the file size.

Is gzipping even used by Wikimedia? The only mention I could find of it was here:

http://en.wikipedia.org/wiki/MediaWiki_talk:Edittools#New_prototype

This search doesn't bring any better results:

http://meta.wikimedia.org/w/index.php?title=Special%3ASearch&ns0=1&ns12=1&search=gzip+javascript

Wikia, not Wikimedia.
http://images.wikia.com/common/releases_200808.4/skins/monaco/js/allinone_loggedin.js?1212

Can't tell if Wikimedia uses gzip. MediaWiki does internally, though Wikimedia might not have Apache configured for it.
Really it's a general all around good idea to try and enable support for Gzipping.

mhorvath2161 wrote:

I don't think it proper for a feature found on *one* project to trump an enhancement that would be enjoyed universally.

(In reply to comment #8)

I don't think it proper for a feature found on *one* project to trump an
enhancement that would be enjoyed universally.

So you're proposing to disable all the possibilities for other sites to minify their JS, just to make code look better to you? And what if Wikimedia will want to minify next week?

Enjoyed Universally? What kind of source do you have that says that people want semicolons removed from js source?

The actual users of the MediaWiki don't care whether their JS has semicolons in it or not.
But when you remove them, the ones that do care about lint validity and minification do give you shit.

If you're going to normalize code style, do it in a way that the rest of the developer group agrees improves readability.
Personally:

if( test ) {

do this
and then this

} else if( another test )

do this

else

or do this

Is less readable than:

if( test ) {

do this
and then this

} else if( another test )

do this

} else {

or do this

}

And I'm pretty sure that the rest of the group will agree that long lines that span past editor widths look better when broken up.

ayg wrote:

Come on, guys, don't bite the newbies. Be nice.

I'm also against these changes, though. The only arguments in favor are 1) you like the style better and 2) they reduce byte size.

(1) is invalid because a lot of us dislike the style, particularly those of us used to PHP (which is after all what MW is written in). Like a lot of projects, we have a policy not to go around making mass stylistic changes unless they're totally uncontroversial, like changing spaces to tabs or removing trailing whitespace. Not only does the change do nothing, it also will break svn blame, since it changes most of the file: any attempt to see who last changed a particular line will show this commit for some time to come, which is annoying and unnecessary.

(2) has already been rejected as far as any sort of minification goes. We aren't worried about a couple percent less bytes, *especially* if it's gzipped (which negates most of the minification benefit -- although not all of it). We use normal code styles for JavaScript, we don't try to cut bytes for its own sake.

I agree with Dantman's proposed WONTFIX. Nothing against you or the work you did, but as I said, it's unlikely that all of your patches will be accepted.

mhorvath2161 wrote:

(In reply to comment #11)

I agree with Dantman's proposed WONTFIX. Nothing against you or the work you
did, but as I said, it's unlikely that all of your patches will be accepted.

I don't remember you saying this.

ayg wrote:

(bug 15397 comment #8)

1. If some of the changes are accepted but not others (which is likely),

mhorvath2161 wrote:

Woops. Didn't recognize you.

WONTFIX per Daniel and Aryeh.