On 02/02/2015 09:08 AM, Lukas Slebodnik wrote:
[...]
>> Patch 1/5: Note that even under Python 2, `except ValueError,
KeyError:`
>> would not work (it catches ValueError, and stores the exception instance
>> under the name KeyError); please test these changes well.
>>
> It means that in python2 KeyError was not handled at all.
>
> python2 documentation says:
> "If an exception occurs which does not match the exception named in the except
> clause, it is passed on to outer try statements; if no handler is found, it is
> an unhandled exception and execution stops with a message as shown above."
>
> In my patch, you can see that different exception was thrown (ParsingError) in
> handler. I'm not sure hot to test it therefore you are in CC ;-)
> Shall I remove KeyError?
Yes, remove KeyError. Looking at the history, since 2009 the code worked
without catching KeyError (even if that wasn't the intention). I see no
point in adding it now.
Otherwise LGTM
>
>> Patch 4/5: you can use io.StringIO in both 2.6+ and 3.x+
>>
> Yes, but...
> Traceback (most recent call last):
> File "../src/sbus/sbus_codegen", line 931, in <module>
> main()
> File "../src/sbus/sbus_codegen", line 919, in main
> generate_source(parser.parsed_interfaces, filename, options.include)
> File "../src/sbus/sbus_codegen", line 600, in generate_source
> out("/* The following definitions are auto-generated from %s */",
basename)
> File "../src/sbus/sbus_codegen", line 217, in out
> sys.stdout.write(str)
> TypeError: unicode argument expected, got 'str'
>
> So I'm find with such solution for StringIO. The sbus_codegen is python script
> which generate C source file. I do not expect any unicode characters.
> But I'm not opposed better version.
I see.
People could expect "io" to be the io module, so you could write
if sys.version_info[0] > 2:
from io import StringIO
else:
from StringIO import StringIO
...
sys.stdout = buf = StringIO()
... but at this point that's just nitpicking.
>
> Thank you very much for review.
> I squashed some small patches.
>
> LS
I realized there were hardcoded python2 on some places. The 3rd patch should
fix it. I used "env pyhton" for internal python scripts and for tests which
are
not installed.
I had a discussion with Pavel B. and we agreed
that patch "sbus_codegen: Port to python3" needn't be push to upstream
yet.
It would break his other patches. So current version would be used mostly by
downstream.
--
Petr Viktorin