Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@mgedmin better syntax errors #105

Merged
merged 7 commits into from
Feb 7, 2019
Merged

Conversation

facelessuser
Copy link
Owner

Refactored syntax error work done in pull #103

The big difference here is we take control of the output opposed to relying on SyntaxError, which was designed for Python syntax errors, to format it for us. In the future (version 2.0) we will most likely derive from Exception instead of SyntaxError, as it is probably more appropriate for us to not confuse people by deriving from SyntaxError. This will allow us backwards compatibility for exception detection through the 1.0 series.

mgedmin and others added 6 commits February 5, 2019 16:41
This produces tracebacks like the following:

    Traceback (most recent call last):
      ...
      File "/home/mg/src/zopefoundation/zc.catalog/.tox/py37/lib/python3.7/site-packages/zope/testbrowser/browser.py", line 1370, in getControlLabels
        forlbls = html.select('label[for=%s]' % controlid)
      File "/home/mg/src/zopefoundation/zc.catalog/.tox/py37/lib/python3.7/site-packages/bs4/element.py", line 1376, in select
        return soupsieve.select(selector, self, namespaces, limit, **kwargs)
      File "/home/mg/src/soupsieve/soupsieve/__init__.py", line 108, in select
        return compile(select, namespaces, flags).select(tag, limit)
      File "/home/mg/src/soupsieve/soupsieve/__init__.py", line 59, in compile
        return cp._cached_css_compile(pattern, namespaces, flags)
      File "/home/mg/src/soupsieve/soupsieve/css_parser.py", line 192, in _cached_css_compile
        CSSParser(pattern, flags).process_selectors(),
      File "/home/mg/src/soupsieve/soupsieve/css_parser.py", line 930, in process_selectors
        return self.parse_selectors(self.selector_iter(self.pattern), index, flags)
      File "/home/mg/src/soupsieve/soupsieve/css_parser.py", line 772, in parse_selectors
        key, m = next(iselector)
      File "/home/mg/src/soupsieve/soupsieve/css_parser.py", line 917, in selector_iter
        raise SelectorSyntaxError(msg, self.pattern)
      File "<string>", line 1
        label[for=BrowserAdd__zope.catalog.catalog.Catalog]
             ^
    soupsieve.css_parser.SelectorSyntaxError: Malformed attribute selector at position 5

whereas before the traceback ended in

      File "/home/mg/src/zopefoundation/zc.catalog/.tox/py37/lib/python3.7/site-packages/soupsieve/css_parser.py", line 881, in selector_iter
        raise SyntaxError(msg)
      File "<string>", line None
    SyntaxError: Malformed attribute selector at position 5

making it difficult to see what exactly was malformed about the selector.

I've also chosen to introduce an exception subclass
(SelectorSyntaxError), so that CSS parse errors could be distinguished
from genuine Python syntax errors.
Compute the line and column positions for multiline selectors.

Add unit tests.
Now tracebacks that show a syntax error in a multiline selector will
look right:

      File "<string>", line 2
        input
           [type=3]
           ^
    soupsieve.css_parser.SelectorSyntaxError: Malformed attribute selector at position 9

There's perhaps a slight confusion when the error is not on the last
line, because the caret will be shown after the last line, like this:

      File "<string>", line 2
        input
           [type=3]
           [name=foo]
           ^
    soupsieve.css_parser.SelectorSyntaxError: Malformed attribute selector at position 9

I think it's acceptable: if the user pays attention to "line 2", they
can figure it out.

It might be better to change the indentation for the line with the error
to be a little arrow, like this:

      File "<string>", line 2
        input
    -->    [type=3]
           [name=foo]
           ^
    soupsieve.css_parser.SelectorSyntaxError: Malformed attribute selector at position 9

but I thought that might confuse Python traceback parsers, so I didn't
do it.
Copy link
Contributor

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great improvement over my original PR!

offset = 3
text = []
for n, line in enumerate(pattern.split('\n'), start=1):
text.append(('--> {}' if n == self.line else ' {}').format(line))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: skip the --> for the (I expect most common) case where pattern is a single-line string

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea 👍 .

@facelessuser facelessuser added S: approved The pull request is ready to be merged. T: maintenance Maintenance chore. labels Feb 7, 2019
@facelessuser facelessuser added this to the 1.8.0 milestone Feb 7, 2019
@facelessuser facelessuser merged commit 06de57a into master Feb 7, 2019
@facelessuser facelessuser deleted the mgedmin-better-syntax-errors branch February 7, 2019 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: approved The pull request is ready to be merged. T: maintenance Maintenance chore.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants