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

#2953 show a simple and easy error when keyword expressions trigger a syntax error #3121

Merged
merged 8 commits into from
Jan 17, 2018
Merged

#2953 show a simple and easy error when keyword expressions trigger a syntax error #3121

merged 8 commits into from
Jan 17, 2018

Conversation

feuillemorte
Copy link
Contributor

If we use expressions in -k option we get an error if this expression wrong of has python keywords:

-k "foo or import"

AttributeError: Python keyword 'import' not accepted in expressions passed to '-k'

-k "foo or"

AttributeError: Wrong expression passed to '-k': foo or

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Thanks! You might want to use the keyword module instead, though 😉

@feuillemorte
Copy link
Contributor Author

feuillemorte commented Jan 16, 2018

Thanks! You might want to use the keyword module instead, though 😉

fixed =)

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Awesome work!

Just please check if raising UsageError provides a better message.

_pytest/mark.py Outdated
return eval(keywordexpr, {}, mapping)
for kwd in keywordexpr.split():
if keyword.iskeyword(kwd) and kwd not in python_keywords_allowed_list:
raise AttributeError("Python keyword '{}' not accepted in expressions passed to '-k'".format(kwd))
Copy link
Member

Choose a reason for hiding this comment

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

I think raising UsageError might be more appropriate and provide a better user experience because it won't show up as an internal error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@coveralls
Copy link

coveralls commented Jan 16, 2018

Coverage Status

Coverage increased (+0.008%) to 92.622% when pulling dc79116 on feuillemorte:2953-keyword-expressions-error into 01e37fe on pytest-dev:features.

@nicoddemus
Copy link
Member

@feuillemorte I tested this locally and is looking good, thanks! I just pushed a change that makes the error message appear in red to make it more visible. 😉

After this passes I think we can merge it. 👍

@nicoddemus
Copy link
Member

Let's wait for @The-Compiler to take a second look before we merge it. 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 92.622% when pulling e3406e0 on feuillemorte:2953-keyword-expressions-error into 01e37fe on pytest-dev:features.

@The-Compiler
Copy link
Member

LGTM! Travis is currently recovering from an outage, so that'll probably take a bit.

@nicoddemus
Copy link
Member

@The-Compiler thanks for the info. Should we restart the Travis job? Not sure from the outage report if that's something we should attempt or not.

@The-Compiler
Copy link
Member

Hmm, dunno. I'd say let's just wait some more hours since the job apparently wasn't cancelled.

@feuillemorte
Copy link
Contributor Author

@The-Compiler the problem was solved. could you restart it, please?

@The-Compiler
Copy link
Member

I don't think it'll help (as the build wasn't cancelled), but I did.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 92.622% when pulling e3406e0 on feuillemorte:2953-keyword-expressions-error into 01e37fe on pytest-dev:features.

@The-Compiler
Copy link
Member

Looking good now - thanks!

@The-Compiler The-Compiler merged commit 1fd67c9 into pytest-dev:features Jan 17, 2018
@feuillemorte feuillemorte deleted the 2953-keyword-expressions-error branch January 18, 2018 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants