Lint reports do not show the actual problems, only some statistics

[J. Berger <jeberger@…>]

IMO, lint reports should display the actual lint messages the same way that test reports displays tracebacks.

Note that the slave already sends the information to the master, so it's just a question of displaying it.

[osimons]

Yeah, I agree. It would be a nice feature addition. I've also thought about doing something similar to "Coverage" annotation - meaning you could bring up the file and have it annotated marking each line with messages, and the messages displayed in a hovering tooltip or similar.

Good ideas welcome - working patches even more so :-)

[Roland Wilczek <r.wilczek@…>]

could anyone attach a small but expressive sample-XML from pylint?

I am working on providing lint-analysis with the phptools (with PHP_CodeSniffer) and would like to try my hands on bitten's HTML-template too. For this it would be nice to know what information pylint generates.

[osimons]

Regardless of what format a tool generates, the messages are transmitted by the various tools using the same Report Formats. I've just (finally) added the Lint category of reports to this documentation.

For your information, typical pylint output looks like this - and is what <python:pylint/> tool parses into the xml that it then reports:

bitten/report/testing.py:115: [C] Line too long (85/80)
bitten/report/testing.py:11: [W] Wildcard import trac.core
bitten/report/testing.py:17: [C, TestResultsChartGenerator] Missing docstring
bitten/report/testing.py:80: [R, TestResultsSummarizer.render_summary] Too many arguments (6/5)
...

[Roland Wilczek <r.wilczek@…>]

thx.

This helps to understand the implementation of pythontools:pylint.

I will do my very best to provide a PHP-version and to improve the view.

[J. Berger <jeberger@…>]

Replying to Roland Wilczek <r.wilczek@…>:

thx.

This helps to understand the implementation of pythontools:pylint.

I will do my very best to provide a PHP-version and to improve the view.

You can also look at ticket #508 which includes a patch adding support for cppcheck along with an example.

[mpotter@…]

While trying to code up a solution for this ticketI bumped into:

• #547: XML report loading does not load record value.
• #542: Only one summarizer per category

Without a solution for #547 there is no message to display. Without a solution for #542 one would have to pick which summarizer to use, the current summarizer which only gives the numbers or the new detail summarizer.

[hodgestar]

Patch adding lint summary details (based on test failure details)

[hodgestar]

I've added a patch with a basic implementation that could use a little cleaning up. It avoids #547 by changing the pylint upload recipe to put the message in a child element. It uses the same expand / collapse details system as the test failures.

[mpotter@…]

In templates/bitten_summary_lint.html circa line 27 & 28 and report/lint.py circa line 147 I think you wanted 'category' instead of 'type'.

[mpotter@…]

Replying to mpotter@…:

In templates/bitten_summary_lint.html circa line 27 & 28 and report/lint.py circa line 147 I think you wanted 'category' instead of 'type'.

Scratch that, apparently there are more documentation issues with the Lint Reports. I see from code reading that "category", "type", "tag", "line", "file" and now "msg" are set, but the Lint Reports Format does not give "type" and thus for me it fell though to the record type which were all "problem".

[mpotter@…]

Back to being confused. I do not know what you are seeing, but from code reading the python:pylint source​ it appears to me that:

• msg_type is initially a single uppercase character followed by 0 or more numbers: [A-Z]\d*. L139​ L151​
• If msg_type begins with 'W', 'E', 'C' or 'R' then category becomes 'warning', 'error', 'convention', or 'refactor' respectively, otherwise category is None. L142​ L152​
• If msg_type is only a single character then msg_type is changed to None. L153​
• The XML is then generated using the attributes=values: L162​
  • type=msg_type
  • category=category

This would mean that both type and category are optional in the XML. However, since if there is no type value, type gets a value equal to the node name, which is problem. Therefore in the database:

• category is technically optional, but probably is always there with a properly formed pylint source file.
• type is either:
  • A single letter followed by one or more digits: [A-Z]\d+
  • The value problem.

With the example pylint file given above, type would always be problem.

Therefore I am back to: Are we sure we want type to be used in the details, or should we use category or something else (e.g. use type if it is not problem, otherwise use category if available, else use problem`)? Or is my code reading out to lunch?

[J. Berger <jeberger@…>]

Replying to mpotter@…:

Actually, pylint has different formats. The default looks like this:

************* Module pylint.gui
C: 27:LintGui.__init__: Invalid name "txtModule" (should match [a-z_][a-z0-9_]{2,30}$)
W: 56:LintGui.run_lint: Duplicate key 'W:' in dictionary

The example from comment 3 probably came from this command line:

> pylint -r n -f parseable /usr/lib/python2.6/site-packages/pylint/gui.py 
/usr/lib/python2.6/site-packages/pylint/gui.py:27: [C, LintGui.__init__] Invalid name "txtModule" (should match [a-z_][a-z0-9_]{2,30}$)
/usr/lib/python2.6/site-packages/pylint/gui.py:56: [W, LintGui.run_lint] Duplicate key 'W:' in dictionary

To get a format recognized by Bitten, you need to add the -i y option:

> > pylint -i y -r n -f parseable /usr/lib/python2.6/site-packages/pylint/gui.py 
/usr/lib/python2.6/site-packages/pylint/gui.py:27: [C0103, LintGui.__init__] Invalid name "txtModule" (should match [a-z_][a-z0-9_]{2,30}$)
/usr/lib/python2.6/site-packages/pylint/gui.py:56: [W0109, LintGui.run_lint] Duplicate key 'W:' in dictionary

As far as the XML fields are concerned, both category and type should always be there (at least for pylint and cppcheck reports).

• category should properly be called "severity" and tells how important the problem is. Bitten should probably define a set of standard values which all checker plugins must use (cppcheck uses "error", "warning" and "convention" to be as close as possible to pylint);
• type is a tool-specific code designating the precise type of problem. For example, an invalid function name and a missing docstring are both in category "convention", but they have different types (C0103 and C0111 for pylint).

IMO the lint report page should show everything:

• The file and line;
• The severity: "category";
• The error code: "type" (actually that one is the only one that could be omitted);
• The message text.

[J. Berger <jeberger@…>]

Gah, I clicked on the wrong button...

Replying to J. Berger <jeberger@…>:

Replying to mpotter@…:

Actually, pylint has different formats. The default looks like this:

************* Module pylint.gui
C: 27:LintGui.__init__: Invalid name "txtModule" (should match [a-z_][a-z0-9_]{2,30}$)
W: 56:LintGui.run_lint: Duplicate key 'W:' in dictionary

The example from comment 3 probably came from this command line:

> pylint -r n -f parseable /usr/lib/python2.6/site-packages/pylint/gui.py 
/usr/lib/python2.6/site-packages/pylint/gui.py:27: [C, LintGui.__init__] Invalid name "txtModule" (should match [a-z_][a-z0-9_]{2,30}$)
/usr/lib/python2.6/site-packages/pylint/gui.py:56: [W, LintGui.run_lint] Duplicate key 'W:' in dictionary

To get a format recognized by Bitten, you need to add the -i y option:

> pylint -i y -r n -f parseable /usr/lib/python2.6/site-packages/pylint/gui.py 
/usr/lib/python2.6/site-packages/pylint/gui.py:27: [C0103, LintGui.__init__] Invalid name "txtModule" (should match [a-z_][a-z0-9_]{2,30}$)
/usr/lib/python2.6/site-packages/pylint/gui.py:56: [W0109, LintGui.run_lint] Duplicate key 'W:' in dictionary

As far as the XML fields are concerned, both category and type should always be there (at least for pylint and cppcheck reports).

• category should properly be called "severity" and tells how important the problem is. Bitten should probably define a set of standard values which all checker plugins must use (cppcheck uses "error", "warning" and "convention" to be as close as possible to pylint);
• type is a tool-specific code designating the precise type of problem. For example, an invalid function name and a missing docstring are both in category "convention", but they have different types (C0103 and C0111 for pylint).

IMO the lint report page should show everything:

• The file and line;
• The severity: "category";
• The error code: "type" (actually that one is the only one that could be omitted);
• The message text.

[mpotter@…]

Replying to J. Berger <jeberger@…>:
I copied part of your comments to #547.

[mpotter@…]

Comments about the patch as it currently stands:

• The file link takes one to the head version of the file and not the revision that the build was from.
• The message lines could be links that would take one to the specific line.
• There is no indication in the message line of what category of message it is (Convention, Refactor, Warning, or Error). Note: I am not using pylint as the source of my lint message, and for me the type does not reveal the category.
• Layout issue (probably not in the scope of this ticket). With the fixture closed the report looks something like:

   +---------+-----------------------------------------+-------+
   |         | Problem Category Totals                 |       |
   | File    | Convention | Refactor | Warning | Error | Total |
   +---------+------------+----------+---------+-------+-------+
   | > File1 |            |          | 1       | 1     | 2     |
   +---------+------------+----------+---------+-------+-------+
  

  However, when the fixture is opened the messages appear in the same table cell as the file name.

   +------------------------------------------------------------------+-------------...
   |                                                                  | Problem Cate...
   | File                                                             | Convention |...
   +------------------------------------------------------------------+------------+...
   | v File1                                                          |            |...
   | 4: [T1234] A long text message about one of the issues in File1. |            |...
   | 123: [T5432] Another text message about a issues in File1.       |            |...
   +------------------------------------------------------------------+------------+...
  

  It would be better if the text lines opened in a spanning cell below the summary line.

   +-----------+---------------------------------------------+--------+
   |           | Problem Category Totals                     |        |
   | File      | Convention  | Refactor  | Warning  | Error  | Total  |
   +-----------+-------------+-----------+----------+--------+--------+
   | v File1   |             |           | 1        | 1      | 2      |
   +-----------+-------------+-----------+----------+--------+--------+
   | 4: [T1234] A long text message about one of the issues in File1. |
   | 123: [T5432] Another text message about a issues in File1.       |
   +------------------------------------------------------------------+
  

[hodgestar]

Patch updated to apply against r873. Also orders files by name for display.

[osimons]

Note that the report format docs was migrated to repository in [896], so any patch would also need to include changes to documentation.

[Mark Potter <mpotter@…>]

Just got an "AttributeError: 'NoneType' object has no attribute 'isdigit'" on what would be line 152 of bitten/report/lint.py in lint-summary-details-2.diff​. Currently looking into the cause.

[Mark Potter <mpotter@…>]

Replying to Mark Potter <mpotter@…>:

Running the SQL from PyLintSummarizer of lint.py with the problem build number (20343) I get a single record with no values for any of the columns save report_category ('lint').

Running:

SELECT * FROM bitten_report
WHERE build=20343

Returns:

Id	Build	Step	Category	Generator
#13035	20343	Build	lint

The Id of 13035 being the report ID number. Running:

SELECT * FROM bitten_report_item
WHERE report=13035

Returns: nothing

Therefore, I have a report entry but no detail for this report. This makes sense since I just made some changes to my sources that caused all my lint messages to drop to 0 for many of my targets, this report being for one of them. So, questions:

• Should the report line not have been inserted in the first place since there is no details? (probably not)
• Should PyLintSummarizer handle this case where there is a report by no detail records? (probably)

[osimons]

Failing the creation with an assert may do more harm than good. It would make more sense to just add it empty or with some default classification or something. Naturally we should handle empty report lines much more gracefully in the summarizer(s).

Patch with tests welcome :-)

[Mark Potter <mpotter@…>]

Replying to Mark Potter <mpotter@…>:

Taking a hint from coverage.py​, the following appears to resolve the issue:

bitten/report/lint.py

@@ -129 +129 @@
         seen_files = {}
 
         for type, file, line, category, msg, report_category in cursor:
+            try:
+                line = int(line)
+            except TypeError:
+                continue;       # no rows
+
             if not file_data.has_key(file):
                     file_data[file] = {'file': file, 'type': {}, 'lines': 0, 'category': {}, 'details': []}
 
@@ -140 +145 @@
 
             d['lines'] += 1
             line_total += 1
-
-            if line.isdigit():
-                line = int(line)
 
             d['details'].append((line or '??', type or '??', msg or '-'))

[Joel Low <lowjoel@…>]

Replying to Mark Potter <mpotter@…>:

Replying to Mark Potter <mpotter@…>:

Taking a hint from coverage.py​, the following appears to resolve the issue:

bitten/report/lint.py

@@ -129 +129 @@
         seen_files = {}
 
         for type, file, line, category, msg, report_category in cursor:
+            try:
+                line = int(line)
+            except TypeError:
+                continue;       # no rows
+
             if not file_data.has_key(file):
                     file_data[file] = {'file': file, 'type': {}, 'lines': 0, 'category': {}, 'details': []}
 
@@ -140 +145 @@
 
             d['lines'] += 1
             line_total += 1
-
-            if line.isdigit():
-                line = int(line)
 
             d['details'].append((line or '??', type or '??', msg or '-'))

I've attached an updated patch (against 0.6b3) which achieves a few more things. I am not too proficient in Python, so pardon my code (help to improve it would be appreciated!)

1. A link to the file triggering the lint report item (correct to the line number) is created along with each report item.
2. The category of the lint report item is displayed, not the type of report item (which for lint reports always is "lint", so "warning," "error" etc. is now displayed instead)
3. The category results in different highlighting (warning/errors get different CSS styles applied to it for clarity)
4. Lint reports which are empty do not create empty items any more (which may remove the need for Mark Potter's change to the patch -- testing needed)

[Joel Low <lowjoel@…>]

Updated patch against 0.6b3

[osimons]

Nice! Thanks! Patch reads well at least, but haven't tried it. It really is a shame that we don't have any Lint summarizer tests... Hopefully others that use Lint reporting can try the patch and chime in on how it works.

One minor thing I noticed reading through the patch was this CSS tweak: p.details > span. The > used to be something that did not work with IE, but perhaps that is no longer true for any of the newer browsers we support (what ever that list looks like)?

[Joel Low <lowjoel@…>]

Replying to osimons:

One minor thing I noticed reading through the patch was this CSS tweak: p.details > span. The > used to be something that did not work with IE, but perhaps that is no longer true for any of the newer browsers we support (what ever that list looks like)?

The > selector isn't supported until IE7+ (if memory serves) but while I was busy hacking the code through it was the most straightforward way to get the style to work, short of someone more familiar with the DOM of the build report. So if there's a better way (I'm pretty sure there is) please do implement it.

[anonymous]

Replying to osimons:

Yeah, I agree. It would be a nice feature addition. I've also thought about doing something similar to "Coverage" annotation - meaning you could bring up the file and have it annotated marking each line with messages, and the messages displayed in a hovering tooltip or similar.

Good ideas welcome - working patches even more so :-)

An annotation plugin is at ​trac hacks.

[hodgestar]

Patch from lint-summary-details-2.diff updated to apply cleaning against r998 after the fixing of #547.

[hodgestar]

Updated lint summary patch to apply cleanly against r1003 (fixes some bugs shown by tests and separates test and lint CSS too).

[hodgestar]

Summarizer implementation committed in r1004. It's a slightly modified version of the previous patch that adds a Javascript call to enable folding on the expandable rows (I opted to create bitten_lint.js rather than put the three lines of script into the bitten_build.html template).

[mpotter@…]

r1003 and r1004 are not in the 0.6.x branch​ and this ticket is for the 0.6.1 milestone.

[hodgestar]

Thanks for picking this up -- back ported to 0.6.x in r1005.