Skip to content

Conversation

@rnixx
Copy link
Contributor

@rnixx rnixx commented Dec 20, 2017

  • Close open file descriptors in setup.py
  • Some cleanup in restructured text files

Copy link
Member

@loechel loechel left a comment

Choose a reason for hiding this comment

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

There are a few things I would like to see changed, but first of all, thank you for your work to cleanup the documentation.

For a standalone usage:

.. code:: bash
.. code-block:: shell
Copy link
Member

Choose a reason for hiding this comment

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

please use console instead of shell as this is a console session block see: http://pygments.org/docs/lexers/#lexers-for-various-shells


Technical Backgrounds - Links to External Documentation
---------------------------------------------------------
.......................................................
Copy link
Member

Choose a reason for hiding this comment

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

why a level-change of the headings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because it looked like "Technical Backgrounds" is a subsection of "Modifying the AST"

The general workflow to execute Python code that is loaded within a Python program is:

.. testcode::
.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

please do not change .. testcode:: derictives to code-block derictives, as this changes also the behaviour.

code-block only highlights the code,
testcode actualy allows to perform tests on that code

see http://www.sphinx-doc.org/en/stable/ext/doctest.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

With RestrictedPython that workflow should be as straight forward as possible:

.. testcode::
.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

same, keep testcode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

You might also use the replacement import:

.. testcode::
.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

same, keep testcode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

A policy is basically a special ``NodeTransformer`` that could be instantiated with three params for ``errors``, ``warnings`` and ``used_names``, it should be a subclass of RestrictedPython.RestrictingNodeTransformer.

.. testcode:: own_policy
.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

same, keep testcode
please also look at the [group] element of sphinx.ext.doctest the group name defines a context and all tests in this group context are evaluated together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

All ``compile_restricted*`` methods do have a optional parameter ``policy``, where a specific policy could be provided.

.. testcode:: own_policy
.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

same, keep testcode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

That special case would be written as:

.. testcode::
.. code-block:: python
Copy link
Member

Choose a reason for hiding this comment

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

same, keep testcode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

... '''
>>> from RestrictedPython import compile_restricted
>>> code = compile_restricted(src, '<string>', 'exec')
.. code-block:: pycon
Copy link
Member

Choose a reason for hiding this comment

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

could that be a doctest derictive? it is not in sphinx context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean .. code-block:: doctest?

pycon also works well if you view on github. But i can of course change if you want.

Copy link
Member

Choose a reason for hiding this comment

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

pycon is the lexer used by pygments, so just highlighting, I meant replacing code-block:: with doctest::

your restrictions. A usable "safe" set is
``RestrictedPython.Guards.safe_builtins``.

To help illustrate how this works under the covers, here's an example
Copy link
Member

Choose a reason for hiding this comment

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

please do not break documentation lines wthin sentences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

line breaks within sentences are in there all over the place. What's the reason for this policy?

Copy link
Member

@loechel loechel left a comment

Choose a reason for hiding this comment

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

Thanks looks much better

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

@icemac icemac merged commit 66781b9 into zopefoundation:master Feb 6, 2018
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