10000 Fix #251: Python 2 vs Python 3: DevString with bytes by tiagocoutinho · Pull Request #263 · tango-controls/pytango · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Apr 7, 2022. It is now read-only.

Fix #251: Python 2 vs Python 3: DevString with bytes #263

Merged
merged 18 commits into from
Mar 13, 2019
Merged

Fix #251: Python 2 vs Python 3: DevString with bytes #263

merged 18 commits into from
Mar 13, 2019

Conversation

tiagocoutinho
Copy link
Contributor
@tiagocoutinho tiagocoutinho commented Feb 27, 2019 •
edited
Loading

Checked:

  • attributes r/w of types str, [str], [[str]], with bytes/str, both client and server side
  • commands with arg and return types str, [str] with bytes/str, both client and server side
  • properties

@tiagocoutinho tiagocoutinho changed the title Fix #251: Python 2 vs Python 3: DevString with bytes WIP: Fix #251: Python 2 vs Python 3: DevString with bytes Feb 27, 2019
@tiagocoutinho
fix unicode in server commands when not passing through network stack
0eeb1b4
@tiagocoutinho
fix unicode in server attributes when not passing through network stack
< 8000 input type="hidden" name="badge_size" value="small" autocomplete="off" data-targets="batch-deferred-content.inputs" />
81320ae
@tiagocoutinho tiagocoutinho self-assigned this Feb 28, 2019
@tiagocoutinho tiagocoutinho changed the title WIP: Fix #251: Python 2 vs Python 3: DevString with bytes Fix #251: Python 2 vs Python 3: DevString with bytes Feb 28, 2019
ajoubertza
ajoubertza reviewed Feb 28, 2019
View reviewed changes
Copy link
Member
@ajoubertza ajoubertza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improvements.
I'm also wondering if the mentions of UTF-8 in https://github.com/tango-controls/pytango/blob/develop/doc/data_types.rst are still correct after this change?

ext/device_attribute.cpp Outdated
}

static inline bopy::object to_python(const TangoScalarType & value)
{
return bopy::object(std::string(value));
return from_char_to_str2(value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it didn't come in with this PR, but this is a pretty bad name for a function. The difference between the behaviour of from_char_to_str and from_char_to_str2 isn't clear at all.
Looking at the implementation, maybe from_char_to_python_str and from_char_to_boost_str?

Similarly, from_str_to_char isn't quite saying what it does. Seems to create a C++ string from a Python string. Maybe from_python_str_to_char?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree with your naming.

ext/from_py.cpp Outdated
}
return ret;
// DRY!
return from_str_to_char(obj_ptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvement, but now we have a function that just calls another function. Might as well just use from_str_to_char directly. I only see usages of obj_to_new_char in this file, so should be OK to drop it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

ext/from_py.cpp Outdated
}
return ret;
// DRY!
return from_str_to_char(obj_ptr);
}

char* obj_to_new_char(bopy::object obj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the usages are of the the form: obj_to_new_char(py_obj.attr("min_alarm")), so I'm guessing we can probably just remove this boost object variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand what you mean.
by the previous comments I was thinking to obj_to_new_char would be replaced by from_str_to_char. Is that is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's it.

ext/from_py.cpp Outdated
{
result = PyBytes_AsString(obj_ptr);
}
from_str_to_char(obj_ptr, result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, rather drop the obj_to_string function, as use this one elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

ext/from_py.cpp Outdated
{
result = PyBytes_AsString(obj_ptr);
}
from_str_to_char(obj_ptr, result);
}

void obj_to_string(bopy::object obj, std::string& result)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, this variant seems to be unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

tests/test_server.py Outdated
@@ -221,7 +221,7 @@ def bad_attr(self):
# Test properties

def test_device_property_no_default(typed_values, server_green_mode):
dtype, values = typed_values
dtype, values, expected_values = typed_values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expected_values isn't used in this test, but it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this specific test, the expected value is always the default values[0], so here I can remove it.

tests/test_server.py Outdated
@@ -245,7 +245,7 @@ def get_prop(self):


def test_device_property_with_default_value(typed_values, server_green_mode):
dtype, values = typed_values
dtype, values, expected_values = typed_values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, expected_values should probably be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

tests/test_server.py Outdated
@@ -359,7 +359,7 @@ def PolledAttribute3(self):


def test_mandatory_device_property(typed_values, server_green_mode):
dtype, values = typed_values
dtype, values, expected_values = typed_values
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, expected_values should probably be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Makefile
@@ -87,7 +87,7 @@ endif

TANGO_CFLAGS=`pkg-config --cflags-only-other tango`
TANGO_LIBS=`pkg-config --libs-only-l tango`
BOOST_LIB = boost_python-py$(PY_VER_S)
BOOST_LIB = boost_python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change? (I don't know anything about the boost library naming.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, the Makefile commit is a spurious commit.
This was because in my dev env I was using conda and the conda boost package does not suffix the library name with the python version.

I will undo this

@tiagocoutinho
Copy link
Contributor Author

Thanks for improvements.
I'm also wondering if the mentions of UTF-8 in https://github.com/tango-controls/pytango/blob/develop/doc/data_types.rst are still correct after this change?

Yes, you are right. Every mention to UTF-8 should be changed to latin-1

@tiagocoutinho
Copy link
Contributor Author

Thanks for the revision @ajoubertza. It is much appreciated!

@tiagocoutinho tiagocoutinho requested a review from ajoubertza March 8, 2019 16:59
@tiagocoutinho
Copy link
Contributor Author

@ajoubertza, I think I have made all the modifications according to your review.

I have spotted some problems with the database as well. I believe I managed to fix them all.
Unfortunately the C++ FileDatabase which is used in the tests does not support the extended ascii character table in properties (and neither the '\' or the '\n' characters) so I couldn't add tests for this.

If you have time, could you check that everything is ok now?

Thanks

@tiagocoutinho tiagocoutinho changed the title Fix #251: Python 2 vs Python 3: DevString with bytes WIP: Fix #251: Python 2 vs Python 3: DevString with bytes Mar 11, 2019
@tiagocoutinho tiagocoutinho changed the title WIP: Fix #251: Python 2 vs Python 3: DevString with bytes Fix #251: Python 2 vs Python 3: DevString with bytes Mar 11, 2019
ajoubertza
ajoubertza approved these changes Mar 12, 2019
View reviewed changes
Copy link
Member
@ajoubertza ajoubertza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, @tiagocoutinho - thanks. This was a difficult one!
(Sorry for delay in reviewing - I've been on vacation)

tests/test_async.py
eid = proxy.command_inout_asynch('identity', value)
result = proxy.command_inout_reply(eid, timeout=500)
assert_array_equal(result, expected_value)
assert_array_equal(result, expected(value))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice improvement. More readable now, and don't need to do the zip.

tests/test_client.py Outdated
# char \x00 cannot be sent in a DevString. All other 1-255 chars can
ints = tuple(range(1, 256))
bytes_devstring = bytes(ints) if PY3 else ''.join(map(chr, ints))
str_devstring = bytes_devstring.decode('latin-1')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still some duplication here - these things are in test_utils.py.

@tiagocoutinho tiagocoutinho merged commit d61a363 into develop Mar 13, 2019
@ajoubertza ajoubertza deleted the issue-251 branch April 4, 2019 05:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ajoubertza ajoubertza ajoubertza approved these changes

@jairomoldes jairomoldes Awaiting requested review from jairomoldes

Assignees

@tiagocoutinho tiagocoutinho

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tiagocoutinho @ajoubertza
0