Code review comment for lp:~adiroiban/pocket-lint/pocket-lint-css-lint

I know that the current code break the test suite.
I was not sure if such feature is wanted and this is why I did not invest more time into making the test_cssccc tests work together with test_ccc.

----

I am OK with changing it the code to not raise an error on the indented final brace.
This is also the way I prefer it... but from the blog post and readers comment I saw that this formatting is not that popular.

I was thinking to add these two ways of formatting last brace as options and users could decide their format... in a similar way that the opening brace is implemented.
So for pocket-lint you could chose your preferred format.

----

I am new to CSS world. In fact I also wrote this code as a pretext for learning more about CSS.
I am also new to CSSUtils and I don't know what are its weak points.
With a bit of feedback we could also tackler this issues.

-----

I have made some changes bases of what I understood from your comments.

Feel free to add any comments :)

Cheers

Here is the latest diff
----

=== modified file 'pocketlint/contrib/cssccc.py'
--- pocketlint/contrib/cssccc.py 2011-04-17 23:45:04 +0000
+++ pocketlint/contrib/cssccc.py 2011-04-18 01:41:02 +0000
@@ -42,6 +42,7 @@
  * add support for TAB as a separator / identation.
  * add support for @media
 '''
+from __future__ import with_statement

 __version__ = '0.1.0'

@@ -55,17 +56,26 @@
 AT_TEXT_RULES = ['import', 'charset', 'namespace']
 AT_BLOCK_RULES = ['page', 'font-face']
 # If you want
-# selector
+# selector,
+# selector2
 # {
 # property:
 # }
-#IGNORED_MESSAGES = ['I013']
+#IGNORED_MESSAGES = ['I013', 'I014']

 # If you want
+# selector,
 # selector {
 # property:
 # }
-IGNORED_MESSAGES = ['I005']
+#IGNORED_MESSAGES = ['I005', 'I014']
+
+# If you want
+# selector,
+# selector2 {
+# property:
+# }
+IGNORED_MESSAGES = ['I005', 'I006']

 class CSSRule(object):
@@ -218,6 +228,12 @@
                 'I006',
                 'Rule declarations should end with a single new line.',
                 )
+ if last_declaration != '\n ':
+ self.log(
+ start_line + offset,
+ 'I014',
+ 'Rule declarations should end indented on a single new line.',
+ )

 class CSSStatementMember(object):

=== modified file 'pocketlint/formatcheck.py'
--- pocketlint/formatcheck.py 2011-04-17 14:13:31 +0000
+++ pocketlint/formatcheck.py 2011-04-18 01:42:28 +0000
@@ -382,27 +382,37 @@
         if self.text == '':
             return

- if HAS_CSSUTILS:
- handler = CSSReporterHandler(self)
- log = logging.getLogger('pocket-lint')
- log.addHandler(handler)
- parser = cssutils.CSSParser(
- log=log, loglevel=logging.INFO, raiseExceptions=False)
- parser.parseString(self.text)
- log.removeHandler(handler)
-
+ self.check_cssutils()
         self.check_text()
+ # CSS coding conventoins checks should go last since they rely
+ # on previous checks.
+ self.check_css_coding_conventions()
+
+ def check_cssutils(self):
+ """Check the CSS code by parsing it using CSSUtils module."""
+ if not HAS_CSSUTILS:
+ return
+ handler = CSSReporterHandler(self)
+ log = logging.getLogger('pocket-lint')
+ log.addHandler(handler)
+ parser = cssutils.CSSParser(
+ log=log, loglevel=logging.INFO, raiseExceptions=False)
+ parser.parseString(self.text)
+ log.removeHandler(handler)
+
+ def check_text(self):
+ """Call each line_method for each line in text."""
+ for line_no, line in enumerate(self.text.splitlines()):
+ line_no += 1
+ self.check_length(line_no, line)
+ self.check_trailing_whitespace(line_no, line)
+ self.check_conflicts(line_no, line)
+ self.check_tab(line_no, line)
+
+ def check_css_coding_conventions(self):
+ """Check the input using CSS Coding Convention checker."""
         CSSCodingConventionChecker(self.text, logger=self.message).check()

- def check_text(self):
- """Call each line_method for each line in text."""
- for line_no, line in enumerate(self.text.splitlines()):
- line_no += 1
- self.check_length(line_no, line)
- self.check_trailing_whitespace(line_no, line)
- self.check_conflicts(line_no, line)
- self.check_tab(line_no, line)
-

 class PythonChecker(BaseChecker, AnyTextMixin):
     """Check python source code."""

=== modified file 'pocketlint/tests/test_css.py'
--- pocketlint/tests/test_css.py 2011-04-15 03:14:05 +0000
+++ pocketlint/tests/test_css.py 2011-04-18 01:28:19 +0000
@@ -41,7 +41,7 @@
         if not HAS_CSSUTILS:
             return
         checker = CSSChecker('bogus', ill_formed_property, self.reporter)
- checker.check()
+ checker.check_cssutils()
         messages = [
             (3, "CSSValue: No match: 'CHAR', u':'"),
             (0, 'CSSStyleDeclaration: Syntax Error in Property: '
@@ -52,7 +52,7 @@
         if not HAS_CSSUTILS:
             return
         checker = CSSChecker('ballyhoo', invalid_value, self.reporter)
- checker.check()
+ checker.check_cssutils()
         message = (
             'Invalid value for "CSS Color Module Level 3/CSS Level 2.1" '
             'property: speckled: color')

=== modified file 'pocketlint/tests/test_cssccc.py'
--- pocketlint/tests/test_cssccc.py 2011-04-17 23:45:04 +0000
+++ pocketlint/tests/test_cssccc.py 2011-04-18 01:38:45 +0000
@@ -264,20 +264,7 @@
     }
     '''

- ignored_messaged = ['I013']
-
-
-class RuleTesterConventionB(RuleTesterBase):
- '''Class for convention B.
-
- selector1,
- selecter2 {
- property1: value1;
- property2: value2;
- }
- '''
-
- ignored_messaged = ['I005']
+ ignored_messaged = ['I013', 'I014']

 class TestCSSRuleSetSelectorChecksA(RuleTesterConventionA):
@@ -449,6 +436,19 @@
         self.assertEqual('I012', self.last_log.code)

+class RuleTesterConventionB(RuleTesterBase):
+ '''Class for convention B.
+
+ selector1,
+ selecter2 {
+ property1: value1;
+ property2: value2;
+ }
+ '''
+
+ ignored_messaged = ['I005', 'I014']
+
+
 class TestCSSRuleSetSelectorChecksB(RuleTesterConventionB):
     '''Test coding conventions for selector from rule sets.'''

@@ -488,5 +488,42 @@
         self.assertEqual(5, last_log.line_number)

+class RuleTesterConventionC(RuleTesterBase):
+ '''Class for convention C.
+
+ selector1,
+ selecter2 {
+ property1: value1;
+ property2: value2;
+ }
+ '''
+
+ ignored_messaged = ['I005', 'I006']
+
+
+class TestCSSRuleSetDeclarationsChecksC(RuleTesterConventionC):
+ '''Test coding conventions for declarations from rule sets.'''
+
+ def test_valid_declarations(self):
+ stmt = CSSStatementMember(
+ 0, 0, '\n some: 3px;\n other:\n url();\n ')
+ rule = CSSRuleSet(selector=None, declarations=stmt, log=self.log)
+ rule.checkDeclarations()
+ self.assertEqual([], self.logs)
+
+ def test_I014(self):
+ stmt = CSSStatementMember(
+ 0, 0, '\n some: 3px;\n')
+ rule = CSSRuleSet(selector=None, declarations=stmt, log=self.log)
+ rule.checkDeclarations()
+ self.assertEqual('I014', self.last_log.code)
+
+ stmt = CSSStatementMember(
+ 0, 0, '\n some: 3px;\n ')
+ rule = CSSRuleSet(selector=None, declarations=stmt, log=self.log)
+ rule.checkDeclarations()
+ self.assertEqual('I014', self.last_log.code)
+
+
 if __name__ == '__main__':
     unittest_main()