Merge lp://qastaging/~villemvainio/ipython/ipyrender-linuxfix into lp://qastaging/ipython/0.11

Proposed by Ville M. Vainio
Status: Work in progress
Proposed branch: lp://qastaging/~villemvainio/ipython/ipyrender-linuxfix
Merge into: lp://qastaging/ipython/0.11
To merge this branch: bzr merge lp://qastaging/~villemvainio/ipython/ipyrender-linuxfix
Reviewer Review Type Date Requested Status
Fernando Perez Needs Fixing
Brian Granger none Needs Fixing
Review via email: mp+1935@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Brian Granger (ellisonbg) wrote :

Everything looks good, but I do have one question:

I see that you print a warning like this:

print "WARNING: ...."

Is there a reason we don't use the warnings module for this? I have seen this used elsewhere in IPython. Probably should change this code to use it as well unless there is a specific reason.

review: Needs Fixing (none)
Revision history for this message
Fernando Perez (fdo.perez) wrote :

Changes look good overall, we just need a few fixes. I'll put in here comments on the whole set of unmerged revisions rather than one by one.

- Let's start putting extension imports as fully qualified imports. Instead of:

    >>> import ipy_render

use
  from IPython.Extensions import ipy_render.

I expect that we will post 0.10 revisit the idea of loading Extensions wholesale into PYTHONPATH. It was my mistake to allow that to happen back in the days of ipython0/1, since I never expected that to be part of 'ipython1', it can potentially cause too many problems. So let's start having examples already using a long-term valid convention.

- Doctest examples like

    >>> render t_submission_form

need to be put in using IPython prompts instead, since they aren't valid python input. If they are put as IPython prompts, the extended doctest machinery will properly identify them and run them as ipython input.

- The to_clip functions should have always a docstring. If they are going to be defined with if/else constructs, one approach is to do at the end something like

to_clip.__doc__ = """blah..."""

to avoid repetition. But we shouldn't allow *any* function to end up without a docstring moving forward.

- Let's stick to the conventions in docstrings:

  * Single-line summary that is properly self-contained, blank-line, then rest. This is PEP-8 material.

  * Explicitly list inputs and outputs.

  I have the docstrings for toclip and check_cmd in mind, but this applies in general.

- Do these new functions have tests? I don't see them anywhere yet, but I may have missed them.

- Regarding Brian's comment about the 'print warning'. This should at least always use genutils.warn() instead of a manual print statement. This way, we can then switch later genutils.warn() to use the warnings module, without having to hunt print statements all over the code.

review: Needs Fixing
Revision history for this message
Brian Granger (ellisonbg) wrote :

> - Let's start putting extension imports as fully qualified imports. Instead of:
>
> >>> import ipy_render
>
> use
> from IPython.Extensions import ipy_render.

+1

It was very confusing to me the first time I saw this.

> - Doctest examples like
>
> >>> render t_submission_form
>
> need to be put in using IPython prompts instead, since they aren't valid python input. If they are put as IPython prompts, the extended doctest machinery will properly identify them and run them as ipython input.

This is helpful Fernando as none of the rest of us really knows what
the doctest machinery can really do :) Could you add a section on
testing ( I will add stuff on testing Twisted stuff as well) to the
dev sectin of our Sphinx docs.

> - Do these new functions have tests? I don't see them anywhere yet, but I may have missed them.

+1

> - Regarding Brian's comment about the 'print warning'. This should at least always use genutils.warn() instead of a manual print statement. This way, we can then switch later genutils.warn() to use the warnings module, without having to hunt print statements all over the code.

Great, this is good to know. I wasn't aware of genutils.warn().

Cheers,

Brian

Revision history for this message
Fernando Perez (fdo.perez) wrote :

On Sat, Dec 6, 2008 at 3:54 PM, Brian Granger <email address hidden> wrote:

> This is helpful Fernando as none of the rest of us really knows what
> the doctest machinery can really do :) Could you add a section on
> testing ( I will add stuff on testing Twisted stuff as well) to the
> dev sectin of our Sphinx docs.

Will do now. I just got my computer back (hardware problems), I'm on
to the other branch review and will then update and push my own code.

Cheers,

f

Revision history for this message
Brian Granger (ellisonbg) wrote :

Great, I just merged my branch to trunk and it has a lot of docs
updates. Make sure to pull those before making edits to the dev docs.
 Also, one thing that I have changed is that I have started using rst
citations more as Sphinx (as of 0.5) now handles them in a very nice
way in both html and latex.

Cheers,

Brian

On Sat, Dec 6, 2008 at 7:46 PM, Fernando Perez <email address hidden> wrote:
> On Sat, Dec 6, 2008 at 3:54 PM, Brian Granger <email address hidden> wrote:
>
>> This is helpful Fernando as none of the rest of us really knows what
>> the doctest machinery can really do :) Could you add a section on
>> testing ( I will add stuff on testing Twisted stuff as well) to the
>> dev sectin of our Sphinx docs.
>
> Will do now. I just got my computer back (hardware problems), I'm on
> to the other branch review and will then update and push my own code.
>
> Cheers,
>
> f
> --
> https://code.launchpad.net/~villemvainio/ipython/ipyrender-linuxfix/+merge/1935
> You are subscribed to branch lp:ipython.
>

--
Brian E. Granger, Ph.D.
Assistant Professor of Physics
Cal Poly State University, San Luis Obispo
<email address hidden>
<email address hidden>

Revision history for this message
Brian Granger (ellisonbg) wrote :

Any progress on this?

Revision history for this message
Fernando Perez (fdo.perez) wrote :

> Any progress on this?

Curious too...

One important note, that also applies to

https://code.launchpad.net/~villemvainio/ipython/ip0-unittests-1/+merge/1075

Remember to merge from trunk again, to ensure everything is up to date here. A lot has happened in trunk.

Revision history for this message
Fernando Perez (fdo.perez) wrote :

Status? We're getting close to tagging 0.10...

Unmerged revisions

1159. By Ville M. Vainio

only warn once

1158. By Ville M. Vainio

toclip: some simplification / optimization (only run 'which' once)

1157. By Ville M. Vainio

apply ipy_render from Matt Foster

Subscribers

People subscribed via source and target branches