[lttng-dev] [PATCH babeltrace 2/2] CTF writer: Add function to add an integer environment field value

[Simon Marchi]

On 2016-06-19 01:38, Philippe Proulx wrote:
> Thanks for this, it's a great little addition.

Thanks for the feedback!

>> Example:
>> 
>>   w.add_environment_field("foo", 2)
> 
> Not directly related, but an interface like `os.environ` would be nice 
> here
> I think:
> 
>     w.env["foo"] = 2

Sure, not today though :).

>> -        :exc:`ValueError` is raised on error.
>> +        :exc:`ValueError` or `TypeError` is raised on error.
> 
> Use :exc:`TypeError`.

Applied locally.

>> +        if type(name) != str:
> 
> Use `is` operator.

Applied locally:

     if type(name) is not str:

>> +        t = type(value)
> 
> `value_type` might be a better variable name.

It doesn't provide much to use a variable in the first place.  I 
embedded type(value) in the two expressions.

>> +        if t == str:
> 
> Use `is` operator.

Done.

>> +        elif t == int:
> 
> Use `is` operator.

Done.

>> +            ret = 
>> nbt._bt_ctf_writer_add_environment_field_int64(self._w,
>> +                                                                 
>> name,
>> +                                                                 
>> value)
> 
> Could you check that `value`'s value is in the range of the int64_t 
> type,
> so that we catch an illegal operation earlier? Otherwise it's just 
> going to
> "work" an write the wrong value, I guess.

It seems like SWIG does some validation.  Passing an out-of-range value 
throws:


     Traceback (most recent call last):
       File "babeltrace/examples/ctf_writer.py", line 52, in <module>
         writer.add_environment_field("age_of_the_captain", 
9223372036854775807+1)
       File 
"/tmp/babeltrace/lib/python3.5/site-packages/babeltrace/writer.py", line 
2158, in add_environment_field
         value)
     OverflowError: in method 
'_bt_ctf_writer_add_environment_field_int64', argument 3 of type 
'int64_t'


So I think we are covered, unless you'd like if the error message was 
more precise?