Patchwork [6,of,8] import-checker: handle ast.parse SyntaxErrors

login
register
mail settings
Submitter timeless@mozdev.org
Date March 30, 2016, 9:24 a.m.
Message ID <d8be8db8119d2d716ad9.1459329846@waste.org>
Download mbox | patch
Permalink /patch/14183/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

timeless@mozdev.org - March 30, 2016, 9:24 a.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1459325470 0
#      Wed Mar 30 08:11:10 2016 +0000
# Node ID d8be8db8119d2d716ad9ddaaaa077cf278f6273c
# Parent  54b962661f75e4a55c213c0eeb30f5d6024f7dbe
import-checker: handle ast.parse SyntaxErrors
Yuya Nishihara - March 31, 2016, 2:34 p.m.
On Wed, 30 Mar 2016 04:24:06 -0500, timeless wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1459325470 0
> #      Wed Mar 30 08:11:10 2016 +0000
> # Node ID d8be8db8119d2d716ad9ddaaaa077cf278f6273c
> # Parent  54b962661f75e4a55c213c0eeb30f5d6024f7dbe
> import-checker: handle ast.parse SyntaxErrors
> 
> diff --git a/contrib/import-checker.py b/contrib/import-checker.py
> --- a/contrib/import-checker.py
> +++ b/contrib/import-checker.py
> @@ -233,6 +233,13 @@
>  
>  stdlib_modules = set(list_stdlib_modules())
>  
> +def parse(source):
> +    """Wrapper around ast.parse()"""
> +    try:
> +        return ast.parse(source)
> +    except SyntaxError:
> +        return ast.parse("'SyntaxError'")

Doesn't it just bury a SyntaxError? I think it should be reported as an
error like other messages.
Gregory Szorc - April 3, 2016, 6:12 p.m.
On Thu, Mar 31, 2016 at 7:34 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 30 Mar 2016 04:24:06 -0500, timeless wrote:
> > # HG changeset patch
> > # User timeless <timeless@mozdev.org>
> > # Date 1459325470 0
> > #      Wed Mar 30 08:11:10 2016 +0000
> > # Node ID d8be8db8119d2d716ad9ddaaaa077cf278f6273c
> > # Parent  54b962661f75e4a55c213c0eeb30f5d6024f7dbe
> > import-checker: handle ast.parse SyntaxErrors
> >
> > diff --git a/contrib/import-checker.py b/contrib/import-checker.py
> > --- a/contrib/import-checker.py
> > +++ b/contrib/import-checker.py
> > @@ -233,6 +233,13 @@
> >
> >  stdlib_modules = set(list_stdlib_modules())
> >
> > +def parse(source):
> > +    """Wrapper around ast.parse()"""
> > +    try:
> > +        return ast.parse(source)
> > +    except SyntaxError:
> > +        return ast.parse("'SyntaxError'")
>
> Doesn't it just bury a SyntaxError? I think it should be reported as an
> error like other messages.
>
>
Yes, but this patch does get us import-checker.py support for Python 3
without having to fix all syntax errors on Python 3 first.

We may want to establish a standalone test to verify ast parsing on all .py
files (if we don't have one already). test-check-py3-compat.t does this,
but only for Python 3. It seems like a useful test to have. I'm fine with
only 1 test reporting ast/import errors, especially on Python 3 (otherwise
we'd have to update multiple tests when addressing Python 3 compat).
Yuya Nishihara - April 4, 2016, 12:02 p.m.
On Sun, 3 Apr 2016 11:12:16 -0700, Gregory Szorc wrote:
> On Thu, Mar 31, 2016 at 7:34 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Wed, 30 Mar 2016 04:24:06 -0500, timeless wrote:  
> > > # HG changeset patch
> > > # User timeless <timeless@mozdev.org>
> > > # Date 1459325470 0
> > > #      Wed Mar 30 08:11:10 2016 +0000
> > > # Node ID d8be8db8119d2d716ad9ddaaaa077cf278f6273c
> > > # Parent  54b962661f75e4a55c213c0eeb30f5d6024f7dbe
> > > import-checker: handle ast.parse SyntaxErrors
> > >
> > > diff --git a/contrib/import-checker.py b/contrib/import-checker.py
> > > --- a/contrib/import-checker.py
> > > +++ b/contrib/import-checker.py
> > > @@ -233,6 +233,13 @@
> > >
> > >  stdlib_modules = set(list_stdlib_modules())
> > >
> > > +def parse(source):
> > > +    """Wrapper around ast.parse()"""
> > > +    try:
> > > +        return ast.parse(source)
> > > +    except SyntaxError:
> > > +        return ast.parse("'SyntaxError'")  
> >
> > Doesn't it just bury a SyntaxError? I think it should be reported as an
> > error like other messages.
> >
> Yes, but this patch does get us import-checker.py support for Python 3
> without having to fix all syntax errors on Python 3 first.
> 
> We may want to establish a standalone test to verify ast parsing on all .py
> files (if we don't have one already). test-check-py3-compat.t does this,
> but only for Python 3. It seems like a useful test to have. I'm fine with
> only 1 test reporting ast/import errors, especially on Python 3 (otherwise
> we'd have to update multiple tests when addressing Python 3 compat).

I think it's good thing to catch the SyntaxError, but IMHO it should be
reported. Otherwise I would miss the error when running import-checker.py
manually.

Patch

diff --git a/contrib/import-checker.py b/contrib/import-checker.py
--- a/contrib/import-checker.py
+++ b/contrib/import-checker.py
@@ -233,6 +233,13 @@ 
 
 stdlib_modules = set(list_stdlib_modules())
 
+def parse(source):
+    """Wrapper around ast.parse()"""
+    try:
+        return ast.parse(source)
+    except SyntaxError:
+        return ast.parse("'SyntaxError'")
+
 def imported_modules(source, modulename, localmods, ignore_nested=False):
     """Given the source of a file as a string, yield the names
     imported by that file.
@@ -291,7 +298,7 @@ 
     ['foo.__init__']
     """
     fromlocal = fromlocalfunc(modulename, localmods)
-    for node in ast.walk(ast.parse(source)):
+    for node in ast.walk(parse(source)):
         if ignore_nested and getattr(node, 'col_offset', 0) > 0:
             continue
         if isinstance(node, ast.Import):
@@ -338,7 +345,7 @@ 
     The legacy convention only looks for mixed imports. The modern convention
     is much more thorough.
     """
-    root = ast.parse(source)
+    root = parse(source)
     absolute = usingabsolute(root)
 
     if absolute: