Minor code cleanup #34
Conversation
| @@ -275,7 +275,7 @@ def metric_payload(self, value, min_value=None, max_value=None, count=None, | |||
| if not value: | |||
| value = 0 | |||
|
|
|||
| if isinstance(value, basestring): | |||
| if isinstance(value, ("".__class__, u"".__class__)): | |||
There was a problem hiding this comment.
Wouldn't it be better to use six for this? I find this method a tad hard to read to be honest :P
if isinstance(value, six.string_types)
There was a problem hiding this comment.
I thought about that ... but didn't want to introduce another library dependency. Though, as we move to trying to make this fully python3 compliant, maybe we'll rely on six for other stuff. Would you rather I back this out? or introduce six as a dependency?
There was a problem hiding this comment.
Personally I think six is available nearly everywhere because of other dependencies so it shouldn't be that much of a problem, but lets ask @Rockstar04 as well :)
Alternatively we could simply integrate six into the library since it's a single file anyhow: https://github.com/benjaminp/six/blob/master/six.py
There was a problem hiding this comment.
I'm pretty sure I've mentioned before, I'm from from familiar with the Python ecosystem. I do know enough that python-six is available all the way back to Ubuntu 12.04, and a quick search looks like good coverage in RPM based systems as well. I'd be fine with adding it as a dependency. We would just have to make sure its clearly listed in the readme here.
There was a problem hiding this comment.
OK, I updated it to use six and included six>=1.5 in setup.py. wasn't sure what version, but awscli uses six>=1.5 so that seemed good to me.
No description provided.