Merge lp:~villemvainio/ipython/trunk-dev into lp:ipython/0.11

Proposed by Ville M. Vainio
Status: Needs review
Proposed branch: lp:~villemvainio/ipython/trunk-dev
Merge into: lp:ipython/0.11
To merge this branch: bzr merge lp:~villemvainio/ipython/trunk-dev
Reviewer Review Type Date Requested Status
Fernando Perez Needs Fixing
IPython Developers Pending
Review via email: mp+3620@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Ville M. Vainio (villemvainio) wrote :

Few hooks & small fixes

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

-r1153: I'm concerned about deeper and deeper hooks going in for everything. Perhaps we want to have an on-list discussion on this, because this is adding a triple try/except and a function call in the key code execution point. This change can NOT go in without further on-list discussion and testing for its possible impacts. The idea may be sound, but we need to sort out with everyone else this design. Please start a thread on list to sort this one out.

-r1155: No example/doctest in the docstring.

-r1156: No example/doctest in the docstring.

-r1159: no tests. A test needs to be added (even if there wasn't one before) for that fails with the previous code and works with the current one.

- r1160: the exception should be reported with the main exception handling mechanism, not a naked 'print' that goes to stdout.

  Also, the 'return True' can then just be outside the try/except, to avoid repetition.

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

Any progress on this one? We're getting close to tagging 0.10...

lp:~villemvainio/ipython/trunk-dev updated
1161. By Ville M. Vainio <ville@debian>

examples to docstrings

1162. By Ville M. Vainio <ville@debian>

reverted greedycompleter change (pending test case)

Revision history for this message
Ville M. Vainio (villemvainio) wrote :

I addressed the missing comments (1155, 1156). I also reverted 1159 (it doesn't make sense, IMO, to add a micro-test for this particular completer - it should be a part of proper completer test suite that tests the whole interactive completion chain).

r1160 - I'm hesitant to do full-blown exception display for this scenario, because it deals with ctrl+c and gthread (it's always a trivial IOError, I think). This will never display an exception that is of any use for the user, and it makes sense to mimize the stuff we run in these corner cases

Apart from that, it should be safe for merging now.

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

I will try to review this later today.

Brian

On Wed, Apr 15, 2009 at 2:33 AM, Ville M. Vainio <email address hidden> wrote:
> I addressed the missing comments (1155, 1156). I also reverted 1159 (it doesn't make sense, IMO, to add a micro-test for this particular completer - it should be a part of proper completer test suite that tests the whole interactive completion chain).
>
> r1160 - I'm hesitant to do full-blown exception display for this scenario, because it deals with ctrl+c and gthread (it's always a trivial IOError, I think). This will never display an exception that is of any use for the user, and it makes sense to mimize the stuff we run in these corner cases
>
> Apart from that, it should be safe for merging now.
> --
> https://code.launchpad.net/~villemvainio/ipython/trunk-dev/+merge/3620
> 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 :

-r1153:

Fernando recommended that this change does not go in until we have a broader discussion about hooks in the core. I don't remember this happening. What is the status of this?

-r1154:

This involves more hooks, and this should probably be included in the above discussion.

-r1155 and r1156:

Doctests have been added in r1161, but can doctest handle these prompts:

[~]|3> uniq([1,1,1,2,3,1,3])
<3> [1, 2, 3]

-r1160:

I don't think a raw print is a good idea and this should be addressed.

lp:~villemvainio/ipython/trunk-dev updated
1163. By Ville M. Vainio

merge from trunk

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

Ville, do you have any plans to work on this one further?

I've merged everything else that had been cleaned up. Or do you intend to work further on them and update them for merging into 0.11 with more time? Note that will probably require some manual labor, since when we merge the refactoring work into trunk lots of things will move around.

Revision history for this message
Ville M. Vainio (villemvainio) wrote :

On Sat, Aug 1, 2009 at 12:38 AM, Fernando Perez<email address hidden> wrote:

> Ville, do you have any plans to work on this one further?

No, I think this is now as done as it will ever get & could be merged.

--
Ville M. Vainio
http://tinyurl.com/vainio

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

On Sat, Aug 1, 2009 at 12:06 AM, Ville M. Vainio<email address hidden> wrote:
> No, I think this is now as done as it will ever get & could be merged.

OK, it might be better to remove the merge proposal for now then,
until the code is updated and it can be merged later, if you pull
pieces of the code up again.

Revision history for this message
Ville M. Vainio (villemvainio) wrote :

On Sun, Aug 2, 2009 at 9:36 AM, Fernando Perez<email address hidden> wrote:

> OK, it might be better to remove the merge proposal for now then,
> until the code is updated and it can be merged later, if you pull
> pieces of the code up again.

I meant that it should be ready to merge, and that I don't plan
further work on it. It's not clear to me what is missing from this
branch in the first place.

--
Ville M. Vainio
http://tinyurl.com/vainio

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

> I meant that it should be ready to merge, and that I don't plan
> further work on it. It's not clear to me what is missing from this
> branch in the first place.

There are at least Brian's comments that have not been addressed since his last review.

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

> I meant that it should be ready to merge, and that I don't plan
> further work on it. It's not clear to me what is missing from this
> branch in the first place.

There are at least Brian's comments that have not been addressed since his last review.

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

> > I meant that it should be ready to merge, and that I don't plan
> > further work on it. It's not clear to me what is missing from this
> > branch in the first place.
>
> There are at least Brian's comments that have not been addressed since his
> last review.

Ville, would you like to address these? If I'm correct this branch applies to the 0.10 series, and I'm OK merging it for 0.10.1 if you can reply to Brian's comments. But I don't want to set a precedent of merging code when the review process leaves unhandled issues.

Unmerged revisions

1163. By Ville M. Vainio

merge from trunk

1162. By Ville M. Vainio <ville@debian>

reverted greedycompleter change (pending test case)

1161. By Ville M. Vainio <ville@debian>

examples to docstrings

1160. By Ville M. Vainio

Fix crash with Windows and -gthread on ctrl+c
Contributed by Pieter Cristiaan de Groot

1159. By Ville M. Vainio

greedycompleter: ignore ... = prefix, to allow a = foo[0].<TAB> syntax. Request by Darren Dale

1158. By Ville M. Vainio

fix [Bug 322393] [NEW] ipy_greedycompleter.py need readline fix for win32

1157. By Ville M. Vainio

exec perms (+x) to some scripts (to obsolete binary-fixup in debian rules)

1156. By Ville M. Vainio

__getslice__ for SList

1155. By Ville M. Vainio

always ensure that list of matches is unique (also with custom completers)

1154. By Ville M. Vainio

new hooks: get_line_endidx, get_line_buffer

actually use non-full line buffer for custom completer dispatching (works in the middle of the line)

Subscribers

People subscribed via source and target branches