On 02/10/2015 11:36 PM, Lukas Slebodnik wrote:
On (02/02/15 13:32), Petr Viktorin wrote:
> 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.
>
Removed
> 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()
>
Fixed
> ... 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.
>
Thank you very much for review.
Updated version is attached.
LGTM
--
Petr Viktorin