Merge lp://staging/~adiroiban/pocket-lint/1168854-google-closure into lp://staging/pocket-lint

The tests could have a guard at the top. I think some css tests do this...and I see your function does this. The test can do the same.

I am concerned about the change on lines 40-41. I added this to test under python3. Did the tests pass under py2 and py3 after this change?

What does closure_linter provide that jslint does not? Am I going to see duplicate warnings and errors? Do jsline and closure_lint disagree about style, making it impossible to write without warngings? I think this provides jsdoc checking too. http://anton.averin.pro/2012/08/17/js_quality_tools_jshint_jslint_gjslinter/ implies there could be disagreement, and that it is not going to catch the errors that jslint does.

changes for 40-41 should be reverted... It is my error. I have not run the test.

This code is not correct as it was just a prototype. My local version is a mess but will push a new version.

Some errors will be duplicated (ex long lines)... I plan to white list by default.

I like closure_lint since it is pure python and on Windows it works out of the box.
JSLint/JSHint is nice, but it requires a JS interpreter... and I don't know if right now pocket-lint can use JSLint out of the box.

For now I use both JSHint and closure linter without any conflict...but I guess that users will choose between one js checker and will not use both.

Beside full python support, I like closure linter since it does a some more strict checks on styleguide to make sure that the code look like being written by a single person.

For example closure linter will make a different this code

"""
var test = function(){
}
"""

and this one

"""
var test = function() {
}
"""

While JSHint has noting to comment.

closure linter also complai about missing space between operators so will complain for code like """var a=b;""" as it wants """var a = b;"""

-----

The purporse of this initial review is to see if you want google closure linter. If you want to have support for closure linter, I will come back with a new branch with tests.

Now, I/we can also think of a plugin system for pocketlint so that I (others) can implement 3rd party checkers.

Cheers,

I like your point about windows...or the reverse...the js engine is not universal. Users are unlikely to have both tools available, so there is little chance of a conflict. I was pondering supporting nodejs, but I think your work addresses some of the need. Let's proceed with supporting closure.

Hi Curtis,

Sorry for the delay.

Here is the latest branch with included tests.

Since the initial review request, I have continuously used Google closure linter, and I am happy with it.

-----

I think that many JS projects will still use nodejs, since in these days is very hard to develop something in JS without nodejs.

----

Please check changes and let me know what needs to be changed.

Thanks!

There is an issue that needs to be addresses before I can review closure again. I think line 43 breaks py3 compatibility.

python3 ./test.py
.................................................................................................................................................................................ssE....................
======================================================================
ERROR: test_invalid_value (tests.test_javascript.TestJavascript)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/curtis/Hacking/pocket-lint/pocketlint/tests/test_javascript.py", line 59, in test_invalid_value
    checker.check()
  File "/home/curtis/Hacking/pocket-lint/pocketlint/formatcheck.py", line 708, in check
    self.check_jslint()
  File "/home/curtis/Hacking/pocket-lint/pocketlint/formatcheck.py", line 724, in check_jslint
    line_no, char_no_, message = issue.split('::')
TypeError: Type str doesn't support the buffer API

----------------------------------------------------------------------
Ran 200 tests in 0.154s

FAILED (errors=1, skipped=2)

Hi,

Thanks for the review.

What versions of Python do you want to support?

I tried with 3.3 and got the following errors:

$ python test.py
...........F.............................................................................E.............................................................................ss.......................
======================================================================
ERROR: test_zpt_without_namespace (tests.test_xml.TestXML)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/adi/vcs/pocketlint-branches/1168854-google-closure/pocketlint/tests/test_xml.py", line 90, in test_zpt_without_namespace
    checker.check()
  File "/home/adi/vcs/pocketlint-branches/1168854-google-closure/pocketlint/formatcheck.py", line 433, in check
    self.handle_namespaces(parser)
  File "/home/adi/vcs/pocketlint-branches/1168854-google-closure/pocketlint/formatcheck.py", line 418, in handle_namespaces
    xparser.DefaultHandlerExpand = parser._default
AttributeError: 'xml.etree.ElementTree.XMLParser' object has no attribute '_default'

======================================================================
FAIL: test_code_with_warnings (tests.test_python.TestPyflakes)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/adi/vcs/pocketlint-branches/1168854-google-closure/pocketlint/tests/test_python.py", line 140, in test_code_with_warnings
    self.reporter.messages)
AssertionError: Lists differ: [(3, "undefined name 'b'"), (3... != []

First list contains 2 additional elements.
First extra element 0:
(3, "undefined name 'b'")

+ []
- [(3, "undefined name 'b'"),
- (3, "local variable 'a' is assigned to but never used")]

----------------------------------------------------------------------
Ran 192 tests in 1.368s

FAILED (failures=1, errors=1, skipped=2)

-----

In Python 3.2 I got the following errors:

$ python test.py
...........F..................................................................................F.......................................................................E.............
======================================================================
ERROR: tests.test_javascript (unittest.loader.ModuleImportFailure)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.2/unittest/case.py", line 370, in _executeTestPart
    function()
  File "/usr/lib/python3.2/unittest/loader.py", line 32, in testFailure
    raise exception
ImportError: Failed to import test module: tests.test_javascript
Traceback (most recent call last):
  File "/usr/lib/python3.2/unittest/loader.py", line 256, in _find_tests
    module = self._get_module_from_name(name)
  File "/usr/lib/python3.2/unittest/loader.py", line 234, in _get_module_from_name
    __import__(name)
  File "/home/adi/vcs/pocketlint-branches/1168854-google-closure/pocketlint/tests/test_javascript.py", line 76
    [(1, u'E:0002: Missing space before "+"')],
                                           ^
SyntaxError: invalid syntax

============================...

I am using 2.7 and 3.3 on saucy (and trusty) I *did* fix some py3 issues a few months ago when I went from py3.2 to 3.3. The XML error probably relates py3.2. The XML parsing rules were getting too difficult to maintain across several versions; I introduced a custom parser that does the minimum needed to check well-formedness, comment entities, and namespaces.

curtis@retrograde:pocket-lint$ python3.3 ./test.py
......................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 198 tests in 0.206s

OK
curtis@retrograde:pocket-lint$ python2.7 ./test.py
......................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 198 tests in 0.150s

OK

I will look at this on the plane. I need to remove jslint because the license is not free. It forces pocketlint to be in multiverse.

I merged, fixed a bytestring to str error, saw all tests pass except for the two closure tests that were skipped. I assume the closure tests work for you so I merged into trunk. I will follow up on closure as a jslint replacement when I get more network.