[hawkmoth] Re: [PATCH v2 2/3] hawkmoth: add support for propagating diagnostics

  • From: Marius Vlad <marius.vlad@xxxxxxxxxxxxx>
  • To: hawkmoth@xxxxxxxxxxxxx
  • Date: Wed, 1 May 2019 15:46:34 +0300

On Wed, May 01, 2019 at 02:01:59PM +0200, Bruno Santos wrote:

On 12:21:20 2019-05-01, Marius Vlad wrote:
Thank so much for taking care of these! 

Have a few of small questions bellow:

On Wed, May 01, 2019 at 12:28:37AM +0200, Bruno Santos wrote:
At the same time, the first diagnostics are implemented or rather
forwarded from libclang parse.

Original concept: Marius Vlad
---
 hawkmoth/__init__.py  | 22 ++++++++++++++++++++--
 hawkmoth/__main__.py  |  6 +++++-
 hawkmoth/parser.py    | 27 ++++++++++++++++++++++++++-
 test/test_hawkmoth.py |  2 +-
 4 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/hawkmoth/__init__.py b/hawkmoth/__init__.py
index 3355922..0896453 100644
--- a/hawkmoth/__init__.py
+++ b/hawkmoth/__init__.py
@@ -21,7 +21,7 @@
 from sphinx.util.docutils import switch_source_input
 from sphinx.util import logging
 
-from hawkmoth.parser import parse
+from hawkmoth.parser import parse, ErrorLevel
 
 with open(os.path.join(os.path.abspath(os.path.dirname(__file__)),
                        'VERSION')) as version_file:
@@ -42,13 +42,31 @@ class CAutoDocDirective(Directive):
     }
     has_content = False
 
+    # Map verbosity levels to logger levels.
+    _log_lvl = {ErrorLevel.ERROR: logging.LEVEL_NAMES['ERROR'],
+                ErrorLevel.WARNING: logging.LEVEL_NAMES['WARNING'],
+                ErrorLevel.INFO: logging.LEVEL_NAMES['INFO'],
There's no ErrorLevel.INFO in that parser Enum. Do we care about it?

Hey,

INFO is there. Am I missing something?
Nope, mea culpa, somehow I've missed it!


+                ErrorLevel.DEBUG: logging.LEVEL_NAMES['DEBUG']}
+
+    def __display_parser_diagnostics(self, errors):
+        env = self.state.document.settings.env
+
+        for (severity, filename, lineno, msg) in errors:
+            toprint = '{}:{}: {}'.format(filename, lineno, msg)
+
+            if severity.value <= env.app.verbosity:
This should be strict such that in case there's no verbosity (0) it
doesn't match of equal to 0 severity value of ERROR.

This is intentional. Errors should always be shown, warnings must be
enabled and further levels require further flags. Seems reasonable to
me.

Point of reference, calling gcc always shows errors, it's the warnings
that start out disabled by default.

Roger! Okay sounds fine to me!


+                self.logger.log(self._log_lvl[severity], toprint,
+                                location=(env.docname, self.lineno))
+
     def __parse(self, viewlist, filename):
         env = self.state.document.settings.env
 
         compat = self.options.get('compat', env.config.cautodoc_compat)
         clang = self.options.get('clang', env.config.cautodoc_clang)
 
-        comments = parse(filename, compat=compat, clang=clang)
+        comments, errors = parse(filename, compat=compat, clang=clang)
+
+        self.__display_parser_diagnostics(errors)
 
         for (comment, meta) in comments:
             lineoffset = meta['line'] - 1
diff --git a/hawkmoth/__main__.py b/hawkmoth/__main__.py
index 1271772..1efb267 100644
--- a/hawkmoth/__main__.py
+++ b/hawkmoth/__main__.py
@@ -31,11 +31,15 @@ def main():
                         help='Verbose output.')
     args = parser.parse_args()
 
-    docs = parse(args.file, compat=args.compat, clang=args.clang)
+    docs, errors = parse(args.file, compat=args.compat, clang=args.clang)
 
     for (doc, meta) in docs:
         if args.verbose:
             print('# {}'.format(meta))
         print(doc)
 
+    for (severity, filename, lineno, msg) in errors:
+        print('{}: {}:{}: {}\n'.format(severity.name,
+                                       filename, lineno, msg), 
file=sys.stderr)
+
 main()
diff --git a/hawkmoth/parser.py b/hawkmoth/parser.py
index ecb0d24..60002bc 100644
--- a/hawkmoth/parser.py
+++ b/hawkmoth/parser.py
@@ -33,6 +33,7 @@
 Otherwise, documentation comments are passed through verbatim.
 """
 
+import enum
 import itertools
 import sys
 
@@ -43,6 +44,16 @@
 
 from hawkmoth.util import docstr, doccompat
 
+class ErrorLevel(enum.Enum):
+    """
+    Supported error levels in inverse numerical order of severity. The 
values
+    are chosen so that they map directly to a 'verbosity level'.
+    """
+    ERROR = 0
+    WARNING = 1
+    INFO = 2
+    DEBUG = 3

Is there a reason not to include all? (FATAL/NOTE)...

The current proposal is:

- ERROR: Something went terribly wrong, documentation is likely broken.

- WARNING: May have affected the documentation output in subtle ways.
  Maybe some symbol is not fully defined and therefore won't be linked.
  Documentation is still sound though.

- INFO: Tangentially related to documentation, but of no consequence to
  the output. E.g. 'entering file xxx', 'found function token', etc.

- DEBUG: Don't affect documentation, but may be important in inspecting
  what the code is doing for debug/development purposes. E.g. some
  lesser error from an external module that we discarded.

I don't see what FATAL and NOTE would bring to the table.

If we really get a lot more errors and must discriminate further, I'd
argue for an horizontal expansion rather than vertical: (e.g.) -wclang,
etc. Adding more levels won't really help in my opinion.

I believe this all make sense, I was merely asking why we don't follow
exactly the clang types, giving the mapping. 

Side-note: Diagnostics from clang will emit a fatal when files are not
found, and error if can't resolve data type.  In our source code,
headers missing combined with function attributes will result in
symbols (functions) not being printed/showed and only the
comment is be present. Currently we tag this as ERROR, yet
documentation is rather sound, but your opinion here might
differ.  Just want to make sure this behaviour is known.


+
 def comment_extract(tu):
 
     # FIXME: How to handle top level comments above a cursor that it 
does *not*
@@ -239,10 +250,22 @@ def _recursive_parse(comments, cursor, nest, 
compat):
 
     return [(doc, meta)]
 
+def clang_diagnostics(errors, diagnostics):
+    sev = {0: ErrorLevel.DEBUG,
+           1: ErrorLevel.DEBUG,
+           2: ErrorLevel.WARNING,
+           3: ErrorLevel.ERROR,
+           4: ErrorLevel.ERROR}
I suppose the same applies here? 4 and 3 are fatal, respective error.

See previous answer.

Cheers

+    for diag in diagnostics:
+        errors.extend([(sev[diag.severity], diag.location.file.name,
+                        diag.location.line, diag.spelling)])
+
 # return a list of (comment, metadata) tuples
 # options - dictionary with directive options
 def parse(filename, **options):
 
+    errors = []
     args = options.get('clang')
     if args is not None:
         args = [s.strip() for s in args.split(',') if len(s.strip()) > 0]
@@ -255,6 +278,8 @@ def parse(filename, **options):
                      TranslationUnit.PARSE_DETAILED_PROCESSING_RECORD |
                      TranslationUnit.PARSE_SKIP_FUNCTION_BODIES)
 
+    clang_diagnostics(errors, tu.diagnostics)
+
     top_level_comments, comments = comment_extract(tu)
 
     result = []
@@ -270,4 +295,4 @@ def parse(filename, **options):
     # Sort all elements by order of appearance.
     result.sort(key=lambda r: r[1]['line'])
 
-    return result
+    return result, errors
diff --git a/test/test_hawkmoth.py b/test/test_hawkmoth.py
index 07c2c14..aa68a9c 100755
--- a/test/test_hawkmoth.py
+++ b/test/test_hawkmoth.py
@@ -11,7 +11,7 @@
 def _get_output(input_filename, **options):
     docs_str = ''
 
-    docs = parse(input_filename, **options)
+    docs, errors = parse(input_filename, **options)
 
     for (doc, meta) in docs:
         docs_str += doc + '\n'
-- 
2.21.0






Attachment: signature.asc
Description: PGP signature

Other related posts: