Convert tool_shed response to unicode

[mvdbeek]

Addresses issue #1702, where strings returned from the toolshed are causing problems in sqlalchemy.
With this PR unpacked json values will be converted to unicode and downstream code that checks instance type is now using isinstance(var, text_types) instead of str or basestring (which is not available in python3).

[nsoranzo]

Unit tests are failing because the 4 parameters are None by default.

[mvdbeek]

Yep, also this is probably not the best place to do the conversion.
I would guess this should be when building the info_dict from the toolshed response.

[nsoranzo]

What happens if a_string is None?

[mvdbeek]

You will get an exception. I copied this function from https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/web/framework/helpers/__init__.py#L101
I was thinking about adding a check for None, but since this is already in use in other places in the code I was thinking to best not change its behaviour.
In fact I think the to_unicode function should be moved out from lib/galaxy/web/framework/helpers/__init__.py ... what do you think about that @nsoranzo ?

[nsoranzo]

It will not raise an exception, it will just return nothing.

Please also keep in mind that everything in lib/galaxy/util/ should be Python3-safe, this function is not (as spotted by py34-lint tox in TravisCI).

[mvdbeek]

It will not raise an exception, it will just return nothing.

ah yes, sorry, I got this mixed up.

Please also keep in mind that everything in lib/galaxy/util/ should be Python3-safe, this function is not (as spotted by py34-lint tox in TravisCI).

Thanks, I will check this.

[nsoranzo]

What about something like:

from six import text_type

def to_unicode(a_string):
    if a_string is None:
        return None
    return text_type(a_string)

[mvdbeek]

Awesome, that's better than

if sys.version_info[0] > 2:
    def unicode(value):
        """
        Passes on value in python3, as all strings are unicode.
        """
        return value

Thanks!

[martenson]

@galaxybot test this

[mvdbeek]

@nsoranzo the remaining failure is in https://github.com/galaxyproject/galaxy/blame/dev/lib/galaxy/tools/parameters/basic.py#L166 .
I see that the comment was added by @jxtx in 2007. Do you think it's safe to remove that conversion ?

[nsoranzo]

This file is not checked for Python3 compatibility yet, but I would suggest to use six.string_types instead of basestring anyway (same applies for lib/galaxy/tools/test.py).

The external parentheses are not needed.

[nsoranzo]

@mvdbeek No idea about that conversion from 2007.

[mvdbeek]

The failing test on os x is just a timeout while fetching wheels.
@nsoranzo
If this is a good/accepted accepted way to handle isinstance(.*, basestring) and explicit calls to unicode in a python 2 and python 3 compatible way, I could also apply this to the whole codebase.

[nsoranzo]

@mvdbeek Restarted the OS X build, now it's ok.

I surely think this is the right approach for the Python 2 & 3 support, but that should be another PR, I guess. I plan to start working on #1715 after finishing #391.

[mvdbeek]

@nsoranzo another PR, I agree. I already have this ... it needs some work to get the import statements at the right place, and to remove calls to unicode.
If it has a chance of being accepted I can work on this.

[nsoranzo]

I think so, but let's ping @jmchilton for confirmation.

[jmchilton]

dev...mvdbeek:replace_basestring Looks good - is that what I am confirming?

[bgruening]

👍

[mvdbeek]

I'm closing this in favour of PR #1731, which also contains the necessary unicode conversion.