Last modified: 2011-03-13 18:05:07 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T17398, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 15398 - Wikibits patch #1 - style and compactness
Wikibits patch #1 - style and compactness
Status: RESOLVED WONTFIX
Product: MediaWiki
Classification: Unclassified
Parser (Other open bugs)
unspecified
All All
: Lowest enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-01 02:09 UTC by SharkD
Modified: 2011-03-13 18:05 UTC (History)
2 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
unified diff file (27.62 KB, application/octet-stream)
2008-09-01 02:10 UTC, SharkD
Details
unified diff file (28.16 KB, patch)
2008-09-01 02:36 UTC, SharkD
Details
unified diff file (29.02 KB, patch)
2008-09-01 03:07 UTC, SharkD
Details

Description SharkD 2008-09-01 02:09:29 UTC
Cosmetic code changes for style and compactness.

1. consistent style is now used throughout the code
2. erred on compactness when multiple styles were used

Apply this patch first.
Comment 1 SharkD 2008-09-01 02:10:10 UTC
Created attachment 5235 [details]
unified diff file
Comment 2 SharkD 2008-09-01 02:36:48 UTC
Created attachment 5243 [details]
unified diff file

Update. Forgot some stuff.
Comment 3 SharkD 2008-09-01 02:38:33 UTC
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.
Comment 4 SharkD 2008-09-01 03:07:09 UTC
Created attachment 5245 [details]
unified diff file

Update. More stuff. The patch is cumulative.
Comment 5 Daniel Friesen 2008-09-01 08:18:04 UTC
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.
Comment 6 SharkD 2008-09-01 15:14:50 UTC
(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
Comment 7 Daniel Friesen 2008-09-01 15:31:20 UTC
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.
Comment 8 SharkD 2008-09-01 16:14:31 UTC
I don't think it proper for a feature found on *one* project to trump an enhancement that would be enjoyed universally.
Comment 9 Max Semenik 2008-09-01 16:50:44 UTC
(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?
Comment 10 Daniel Friesen 2008-09-01 17:15:19 UTC
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.
Comment 11 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-01 19:03:04 UTC
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.
Comment 12 SharkD 2008-09-01 19:11:52 UTC
(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.
Comment 13 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-01 20:27:21 UTC
(bug 15397 comment #8)
> 1) If some of the changes are accepted but not others (which is likely),
Comment 14 SharkD 2008-09-01 21:35:17 UTC
Woops. Didn't recognize you.
Comment 15 Siebrand Mazeland 2008-11-03 00:13:16 UTC
WONTFIX per Daniel and Aryeh.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links