-
Notifications
You must be signed in to change notification settings - Fork 42
Warn when using another Python implementation than CPython. #91
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
Conversation
PyPy cannot be restricted enough by RestrictedPython. Other implementation might have other or similar issues.
| errors = [] | ||
| warnings = [] | ||
| collected_errors = [] | ||
| collected_warnings = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this refactoring have to do with the "not on PyPy" purpose of the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warnings variable shadows the warnings module which is now needed some lines after the variable definition. I did not want to change only the name of one variable.
docs/CHANGES.rst
Outdated
| `tests/verify.py`. | ||
|
|
||
| - Drop support of PyPy as there currently seems to be no way to restrict the | ||
| - Drop support of PyPy as there currently is no way to restrict the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Drop support for PyPy" seems more idiomatic.
| NOT_CPYTHON_WARNING = ( | ||
| 'You are using on a not supported Python implementation.' | ||
| ' Use CPython to prevent security issues when using RestrictedPython!') | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message would read better as:
NOT_CPYTHON_WARNING = (
'RestrictedPython is only supported on CPython: use on other Python '
'implementations may create security issues.'
)|
The Tests for the warning seems not to work in PyPy. @icemac could you have another look. |
This way the second (now deleted test) failed on PyPy.
|
@loechel It worked on CPython because the warning was only emitted once. Are you okay with emitting a warning every time the function is used? |
|
@tseaver Are you okay with my changes? |
… that the warning is thrown
|
@icemac I am ok, with I would like to have the explicite test keeped that runs on the can fail branch pypy in travis-ci. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All questions answered and changes makes sense
PyPy cannot be restricted enough by RestrictedPython.
Other implementation might have other or similar issues.