Skip to content

Conversation

@loechel
Copy link
Member

@loechel loechel commented Feb 4, 2017

No description provided.

@loechel loechel requested a review from icemac February 4, 2017 14:21
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

Thank you for writing documentation! 👍

README.rst Outdated

RestrictedPython is a defined subset of the Python language which allows to provide a program input into a trusted environment.

For full documentation please see docs/index.
Copy link
Member

Choose a reason for hiding this comment

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

I put the documentation on ReadTheDocs: http://restrictedpython.readthedocs.io/en/python3_update/
Please use this link to point to the full documentation.

* `Trello Board`_


.. _`Trello Board`: https://trello.com/b/pKaXJIlT/restrictedpython
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the link to the Trello board as it is not public and not in English. Instead you may refer to the issue tracker here on GitHub.


A few of the action items currently worked on is on our `Trello Board`_.

.. _`Trello Board`: https://trello.com/b/pKaXJIlT/restrictedpython
Copy link
Member

Choose a reason for hiding this comment

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

dito


.. code:: Python
from RestrictedPython import compile_restricted as compile
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the as compile, see next comment why.

pass
"""
byte_code = compile(source_code, filename='<inline code>', mode='exec')
Copy link
Member

Choose a reason for hiding this comment

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

Using compile_restricted here instead of compile shows the difference. And allows easier to understand text in the next paragraph.

Copy link
Member Author

Choose a reason for hiding this comment

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

dismiss as github did not see change from compile to compile_restricted




Also RestrictedPython provides a way to define Policies, by redefining restricted versions of ``print``, ``getattr``, ``setattr``, ``import``, etc..
Copy link
Member

Choose a reason for hiding this comment

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

s/Also/



Also RestrictedPython provides a way to define Policies, by redefining restricted versions of ``print``, ``getattr``, ``setattr``, ``import``, etc..
As shortcutes it offers three stripped down versions of Pythons ``__builtins__``:
Copy link
Member

Choose a reason for hiding this comment

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

s/shortcutes/shortcuts


* ``safe_builtins`` (by Guards.py)
* ``limited_builtins`` (by Limits.py), which provides restriced sequence types
* ``utilities_builtins`` (by Utilities.py), which provides access for standard modules math, random, string and for sets.
Copy link
Member

Choose a reason for hiding this comment

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

They should be importable from RestrictedPython I see no need to name the modules they reside in.

* ``limited_builtins`` (by Limits.py), which provides restriced sequence types
* ``utilities_builtins`` (by Utilities.py), which provides access for standard modules math, random, string and for sets.

There is also a guard function for making attributes immutable --> ``full_write_guard`` (write and delete protected)
Copy link
Member

Choose a reason for hiding this comment

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

I do not think this is correct, the full_write_guard requires special __guarded_*__ for write and delete.

setup.py Outdated
read('CHANGES.rst')),
long_description=(read('README.rst') + '\n' +
read('docs', 'install', 'index.rst') + '\n' +
read('docs', 'usage', 'basic_usage.rst') + '\n' +
Copy link
Member

Choose a reason for hiding this comment

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

If README.rst contains the link to ReadTheDocs these two lines would not be necessary.
BTW they do not contain valid ReST PyPI can understand:

(ERROR/3) Unknown interpreted text role "py:func".

@loechel
Copy link
Member Author

loechel commented Feb 21, 2017

All requested changes applied, but one is not registed by github tool, please new review

Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

LGTM besides some cosmetical change requests.

mode='exec',
policy=policy # Policy Class
)
# exec(byte_code, { ... }, { ... })
Copy link
Member

Choose a reason for hiding this comment

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

This line could be removed.

byte_code = compile_restricted(source_code,
filename='<inline code>',
mode='exec',
policy=policy # Policy Class
Copy link
Member

Choose a reason for hiding this comment

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

In L46 you call the instance policy_instance instead of policy.

* ``utilities_builtins`` (by Utilities.py), which provides access for standard modules math, random, string and for sets.
Describe Guards and predefined guard methods in details

There is also a guard function for making attributes immutable --> ``full_write_guard`` (write and delete protected)
Copy link
Member

@icemac icemac Mar 1, 2017

Choose a reason for hiding this comment

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

I do not think this is correct, the full_write_guard requires special __guarded_*__ methods for write and delete.

@loechel
Copy link
Member Author

loechel commented Mar 2, 2017

@icemac fixed, please new review
I want to merge it, as it is documentation that should be ok, we have to improve it more and more

Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

LGTM.
Thank you four your hard work and your patience with my change requests.

@loechel loechel merged commit 1cad497 into Python3_update Mar 3, 2017
@loechel loechel deleted the more_docs branch March 3, 2017 14:37
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.

3 participants