Python3 compatibility [ESD-1193] #31
Conversation
python/libsettings.pyx
Outdated
| bytes(section), | ||
| bytes(name), | ||
| bytes(str(value))) | ||
| bytearray(section, 'utf8'), |
There was a problem hiding this comment.
When I did the python3 update for the command line settings application, I asked somebody (probably @silverjam ) what the settings key/value encoding should be, and he said ascii. Back then I put it into 2 constants (one for keys and one for values) in the cli application. Maybe getting the encoding (whatever it is) should be part of the public API of libsettings so it can be easily changed if needed and user applications (console and in the future also settings.py) can get it?
Also coordinate this with console's settings PR (at the moment there is one encoding/decoding command that uses hard-coded ascii).
There was a problem hiding this comment.
ascii it is then.
There was a problem hiding this comment.
Need to think about the encoding from API. I am not going to mix it in here anymore though.
There was a problem hiding this comment.
Actually, all these are inputs for the libsettings so the client needs to provide the encoding of the argument string. I need to add the argument for that.
pmiettinen
force-pushed
the
pmiettinen/esd-1193-python3-compatibility
branch
4 times, most recently
from
f32b941 to
ceecf78
Compare
| @@ -13,6 +13,8 @@ | |||
|
|
|||
| from __future__ import absolute_import, print_function | |||
|
|
|||
| from six.moves import input | |||
There was a problem hiding this comment.
should probably add 'client' requirements somewhere. tried running this in a fresh venv and had to install sbp, six and piksi_tools. Not a big deal but would be helpful
There was a problem hiding this comment.
Thanks for testing, I'll keep that in mind.
|
Tested using Swift Console on 2.7.12 and 3.5.0. Settings read/write sanity checks ok. |
python/libsettings.pyx
Outdated
| bytes(str(value))) | ||
| bytearray(section, encoding), | ||
| bytearray(name, encoding), | ||
| bytearray(value, encoding)) |
There was a problem hiding this comment.
isn't there a risk of double encoding with this line and value = str(value).encode(encoding) above?
python/libsettings.pyx
Outdated
| value, | ||
| sizeof(value)) | ||
| if (ret == 0): | ||
| return str(value) | ||
| return value |
There was a problem hiding this comment.
I'm not 100% sure what would be the neatest method but now it seems that all inputs of read/read_all/write are strings but outputs are bytes/bytearrays. Wouldn't it be more symmetric if the outputs were strings, too? I.e.,
return value.decode(encoding) (and the same for read_all)? This way the application wouldn't need to do any decoding in the default uses but an extra encoding parameter (with a default value) would be needed in read_all.
Edit: some values are numeric too, complicating the logic. But I guess they could be returned as strings and the application could convert them to numeric values if needed.
pmiettinen
force-pushed
the
pmiettinen/esd-1193-python3-compatibility
branch
from
f04c073 to
f6f6328
Compare
python/libsettings.pyx
Outdated
| value = bytearray(str(value), 'ascii') | ||
| elif isinstance(value, unicode): | ||
| # python2 can have 'str' or 'unicode' | ||
| value = bytearray(str(value), 'utf8') |
There was a problem hiding this comment.
copy paste mistake here, need to fix
pmiettinen
force-pushed
the
pmiettinen/esd-1193-python3-compatibility
branch
from
f6f6328 to
4eec11b
Compare
|
Additional bench test; writing an |
| @@ -148,25 +148,38 @@ cdef class Settings: | |||
| def destroy(self): | |||
| settings_destroy(&self.ctx) | |||
|
|
|||
| def write(self, section, name, value): | |||
| def write(self, section, name, value, encoding='ascii'): | |||
There was a problem hiding this comment.
Not very important at this point, and thus this comment can be ignored, but I have a feeling that the encoding could be a server-side setting from application perspective and this library could be the middle-man storing it. The settings change the behavior of the device after all, so the device would know best how it wants to be controlled and what are valid values.
Following this logic, this library could have an additional function/constant that tells the expected encoding, and read/read_all/write wouldn't have an encoding parameter but try to use the expected encoding for string values, throwing an exception if it cannot be done.
However, if there are string settings that accept arbitrary bytes, then this logic wouldn't work well. Then it would be better to take input as bytes/bytearrays. But I am not aware of string settings needing arbitrary bytes.
There was a problem hiding this comment.
There's some merit to this but currently I'd say not relevant since at least on Piksi side everything is (presumed) to be in ascii domain AFAIK.
TODO