minimize.py tool produces incorrect OpenLayers.js file

[bochecha]

When compressing OpenLayers with the minimize.py tool, the produced OpenLayers.js file is incorrect.

First error reported by Firebug is:

missing : after property id
OpenLayers.Control.prototype.initialize.apply(this, arguments);               OpenLayers.js (line 4810)

(others are « OpenLayers is not defined », which most certainly come from the previous one)

I diffed the files produced by mergejs.py and then minimize.py.

The interesting part is there:  http://fpaste.org/XeC5/

As you can see, all multi-line comments and line feeds were removed, making all properties on the same line. But the line also start by the inline comment, which means all properties are commented.

[bochecha]

Ok, looks like the paste expires after some time. Here it is so that it isn't lost.

10721,10785c4809
<   // DOM Elements
<   
<     /**
<      * Property: layersDiv
<      * {DOMElement} 
<      */
<     layersDiv: null,
<     
<     /** 
<      * Property: baseLayersDiv
<      * {DOMElement}
<      */
<     baseLayersDiv: null,
< 
<     /** 
<      * Property: baseLayers
<      * {Array(<OpenLayers.Layer>)}
<      */
<     baseLayers: null,
<     
<     
<     /** 
<      * Property: dataLbl
<      * {DOMElement} 
<      */
<     dataLbl: null,
<     
<     /** 
<      * Property: dataLayersDiv
<      * {DOMElement} 
<      */
<     dataLayersDiv: null,
< 
<     /** 
<      * Property: dataLayers
<      * {Array(<OpenLayers.Layer>)} 
<      */
<     dataLayers: null,
< 
< 
<     /** 
<      * Property: minimizeDiv
<      * {DOMElement} 
<      */
<     minimizeDiv: null,
< 
<     /** 
<      * Property: maximizeDiv
<      * {DOMElement} 
<      */
<     maximizeDiv: null,
<     
<     /**
<      * APIProperty: ascending
<      * {Boolean} 
<      */
<     ascending: true,
<  
<     /**
<      * Constructor: OpenLayers.Control.LayerSwitcher
<      * 
<      * Parameters:
<      * options - {Object}
<      */
<     initialize: function(options) {
---
>   // DOM ElementslayersDiv: null,baseLayersDiv: null,baseLayers: null,dataLbl: null,dataLayersDiv: null,dataLayers: null,minimizeDiv: null,maximizeDiv: null,ascending: true,initialize: function(options) {

This is the part that gets lost by the minimize.py tool.

[bochecha]

Patch 0001-Ticket-2275-minimize.py-tool-produces-incorrect-Open.patch should fix this issue.

[bochecha]

Anyone to review this patch ?

That should be pretty straight-forward and would be of a great help in my packaging of OpenLayers for Fedora.

[elemoine]

Thanks for the patch, it looks good. Some comments:

• in the regex for matching /* */ comments, why not

p = re.compile(r'\s*/\*.*?\*/', re.DOTALL)

instead of

p = re.compile(r'\s+/\*.*?\*/', re.DOTALL)

• in the regex for matching // comments, why not

p = re.compile(r'^\s*//.*$', re.MULTILINE)

instead of

p = re.compile(r'^\s*//.*\s*$', re.MULTILINE)

?

[bochecha]

Replying to elemoine:

Thanks for the patch, it looks good. Some comments: * in the regex for matching /* */ comments, why not {{{ p = re.compile(r'\s*/\*.*?\*/', re.DOTALL) }}} instead of {{{ p = re.compile(r'\s+/\*.*?\*/', re.DOTALL) }}}

Because if you do that, here is the diff between the file generated by my patch and the file generated with what you propose:

$ diff OpenLayers-minimize-mine.js OpenLayers-minimize-multi.js 
93,95c93
< /* ======================================================================
<     OpenLayers/SingleFile.js
<    ====================================================================== */
---
> 
13377,13382c13375
<             'Accept': 'text/javascript, text/html, application/xml, text/xml, */*',
<             'OpenLayers': true
<         };
<         if (this.method == 'post') {
<             headers['Content-type'] = this.options.contentType +
<                 (this.options.encoding ? '; charset=' + this.options.encoding : '');
---
>             'Accept': 'text/javascript, text/html, application/xml, text/xml, *

As you can see, the */* is matched as opening a comment, and hence some code is removed. That's why I made it mandatory to have at least one space before the opening of the multiline comment. Sure, we miss some comments (like the first part of the diff above which should be removed), but I assumed it was safer.

* in the regex for matching // comments, why not {{{ p = re.compile(r'^{\s*//.*$', re.MULTILINE) }}} instead of {{{ p = re.compile(r'}\s*//.*\s*$', re.MULTILINE) }}}

I figured it would be good to remove as much whitespace as possible after an inline comment, to better minimize the result file. Also, an inline comment could have trailing spaces before the end of the line, so it wouldn't be stripped with the regex you proposed.

[elemoine]

* in the regex for matching // comments, why not

p = re.compile(r'^\s*//.*$', re.MULTILINE)

instead of

p = re.compile(r'^\s*//.*\s*$', re.MULTILINE)

I figured it would be good to remove as much whitespace as possible after an inline comment, to better minimize the result file. Also, an inline comment could have trailing spaces before the end of the line, so it wouldn't be stripped with the regex you proposed.

why wouldn't it? I assumed .* would match 0 or more repetitions of any character, including whitespace characters.

[bochecha]

Replying to elemoine:

I figured it would be good to remove as much whitespace as possible after an inline comment, to better minimize the result file. Also, an inline comment could have trailing spaces before the end of the line, so it wouldn't be stripped with the regex you proposed.

why wouldn't it? I assumed .* would match 0 or more repetitions of any character, including whitespace characters.

Not exactly. Whitespace characters can be the spaces, but also new lines. As re.DOTALL is not used, new lines are not covered by ".*".

So adding this "\s*" allows to remove new lines after the inline comment as well.

Of course, that would be covered by strip_empty_lines_helper(), so it's probably simpler to remove the "\s*" and let the second helper take care of it. :)