For patch submissions and discussion

Meador Inge | 1 Nov 2011 15:06

Re: [python] [patch] PR python/13345

On 10/31/2011 10:51 AM, Phil Muldoon wrote:

> This patch fixes a case where the tilde (~) command was being passed to
> Python via the "source" command.  Python does not understand what to do
> with a tilde, so we have to expand it first.

While I can't give an OK, I did review this and was able to reproduce
the stated problem and the patch fixes it.  So, LGTM.  What about
a test case, though?  You could construct a relative path to a
test directory from '~/'.

--

-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software

headers

Phil Muldoon | 1 Nov 2011 15:14

Re: [python] [patch] PR python/13345

Meador Inge <meadori <at> codesourcery.com> writes:

> On 10/31/2011 10:51 AM, Phil Muldoon wrote:
>
>> This patch fixes a case where the tilde (~) command was being passed to
>> Python via the "source" command.  Python does not understand what to do
>> with a tilde, so we have to expand it first.
>
> While I can't give an OK, I did review this and was able to reproduce
> the stated problem and the patch fixes it.  So, LGTM.  What about
> a test case, though?  You could construct a relative path to a
> test directory from '~/'.

The only addition in the patch was tilde_expand, and an additional error
check.  tilde_expand is a readline function.  So we would be testing
that, more or less.  I do normally write regression tests, but I felt
for this one it was not necessary as the patch is somewhat trivial.
Plus I am not sure how constructing a path with a ~ in it would work on
mingw builds?  If so, and we really do want one, I can attempt to write
one.

Cheers

Phil

headers

Meador Inge | 1 Nov 2011 19:05

Re: [python] [patch] PR python/13345

On 11/01/2011 09:14 AM, Phil Muldoon wrote:
> Meador Inge <meadori <at> codesourcery.com> writes:
> 
>> On 10/31/2011 10:51 AM, Phil Muldoon wrote:
>>
>>> This patch fixes a case where the tilde (~) command was being passed to
>>> Python via the "source" command.  Python does not understand what to do
>>> with a tilde, so we have to expand it first.
>>
>> While I can't give an OK, I did review this and was able to reproduce
>> the stated problem and the patch fixes it.  So, LGTM.  What about
>> a test case, though?  You could construct a relative path to a
>> test directory from '~/'.
> 
> The only addition in the patch was tilde_expand, and an additional error
> check.  tilde_expand is a readline function.  So we would be testing
> that, more or less.  I do normally write regression tests, but I felt
> for this one it was not necessary as the patch is somewhat trivial.
> Plus I am not sure how constructing a path with a ~ in it would work on
> mingw builds?  If so, and we really do want one, I can attempt to write
> one.

That seems reasonable.  I agree the test case can be skipped.  Just figured I
would ask ...

--

-- 
Meador Inge
CodeSourcery / Mentor Embedded

(Continue reading)

headers

Tom Tromey | 1 Nov 2011 18:14

Re: [python] [patch] PR python/13345

>>>>> "Meador" == Meador Inge <meadori <at> codesourcery.com> writes:

Meador> On 10/31/2011 10:51 AM, Phil Muldoon wrote:
>> This patch fixes a case where the tilde (~) command was being passed to
>> Python via the "source" command.  Python does not understand what to do
>> with a tilde, so we have to expand it first.

Meador> While I can't give an OK, I did review this and was able to reproduce
Meador> the stated problem and the patch fixes it.  So, LGTM.  What about
Meador> a test case, though?  You could construct a relative path to a
Meador> test directory from '~/'.

I think in this case it is ok not to have a test.

But if you want to write one it would be fine by me.
It would have to be xfail'd if not under $HOME.

Tom