[WIP] python 3 support #58
Conversation
There was a problem hiding this comment.
I have left some specific inline comments in the current commits highlighting some areas where we could probably tighten things up a bit. One general question for the kids at home is whether or not we can get around importing the whole of six just to fix our exception handling around stringtypes.
I know this is still a WIP PR but better to get comments in early.
| # _interpolate_inner removes references from the refs hash after | ||
| # processing them, so we cannot just iterate the dict | ||
| path, refvalue = self._occurrences.iteritems().next() | ||
| for path, refvalue in self._occurrences.copy().items(): |
There was a problem hiding this comment.
Not clear how this is a 2->3 issue? An explanation on this one would be good.
There was a problem hiding this comment.
iteritems doesn't exist in 3. I'll try what you suggested in the issue though and do iter(self._occurances) instead.
There was a problem hiding this comment.
UGH two-thread comms bane of my existence. No, sorry I meant the removal of the while on line 175.
| @@ -75,15 +76,15 @@ def add_ansible_options_group(parser, defaults): | |||
| apps = data['applications'] | |||
| if options.applications_postfix: | |||
| postfix = options.applications_postfix | |||
| groups.update([(k + postfix, v) for k,v in apps.iteritems()]) | |||
| groups.update([(k + postfix, v) for k,v in apps.items()]) | |||
There was a problem hiding this comment.
So for this (and all like switches to items()) the two do not behave the same in python2 vs python3. I realize that this is the convenient fix but it produces code that behaves fundamentally differently between two versions (and is why six has an iteritems wrapper). You could probably fix this by importing future.utils and keeping iteritems()?
| @@ -115,7 +116,7 @@ def _extend_list(self, cur, new, path): | |||
| ret = [cur] | |||
| offset = 1 | |||
|
|
|||
| for i in xrange(len(new)): | |||
| for i in range(len(new)): | |||
There was a problem hiding this comment.
Much like with items, maybe better to do this with builtings or past than by changing the behavior of the code in python2
There was a problem hiding this comment.
past isn't an existing dependency so pulling that in might not be ideal, but what do you mean by doing this through builtins?
Personally I think that while xrange does perform, len(new) should rarely be so large that it matters.
There was a problem hiding this comment.
The other problem with past is that as far as I can tell, it's not available in all versions of python3
[root@worksrv ~]# python --version
Python 3.5.2
[root@worksrv ~]# python -c 'import past'
Traceback (most recent call last):
File "<string>", line 1, in <module>
ImportError: No module named 'past'
I do have to agree with @Rtzq0 though. In addition to the fact that we should not change any behaviors in order to achieve python3 compatibility, I think it's desirable to ensure that behavior is consistent across runtimes.
The best way to do this would probably be to use xrange as the preferred function and fall back to using range in cases where it is not available. This could be achieved using the check 'xrange' in dir(__builtins__)
There was a problem hiding this comment.
Oof. future is not a builtin module. Ignore part one of that last comment.
| @@ -63,7 +63,7 @@ def test_repr_negations_interspersed(self): | |||
| l = ['a', '~b', 'a', '~d'] | |||
| a = Applications(l) | |||
| is_negation = lambda x: x.startswith(a.negation_prefix) | |||
| GOAL = filter(lambda x: not is_negation(x), set(l)) + filter(is_negation, l) | |||
| GOAL = list(filter(lambda x: not is_negation(x), set(l))) + list(filter(is_negation, l)) | |||
There was a problem hiding this comment.
This is probably the least efficient code to do this. itertools, past, future, or builtins might be better ways to get at this.
There was a problem hiding this comment.
Is that really worth it for what is just test code, operating on very small lists? You'll have to be more specific with your other suggestions, it's not immediately clear to me how any functions in those modules would help out here. I could also replace it with list comprehensions.
|
Just to set up the reference link, this PR is WIP for #57 |
Conflicts: reclass/datatypes/parameters.py setup.py
| @@ -76,7 +76,7 @@ def add_ansible_options_group(parser, defaults): | |||
| apps = data['applications'] | |||
| if options.applications_postfix: | |||
| postfix = options.applications_postfix | |||
| groups.update([(k + postfix, v) for k,v in apps.items()]) | |||
| groups.update([(k + postfix, v) for k,v in six.iteritems(apps)]) | |||
There was a problem hiding this comment.
Why are calls to .items() being replaced with calls to six.iteritems()?
|
Any update on this? |
|
Hey, I'll probably not get to finish this, so I'm just going to close the PR - but please feel free to steal my commits if anyone else wants to continue on it. |
|
We merged similar changes on https://github.com/salt-formulas/reclass |
Fix, usage of numeric keys
Do not merge yet, work in progress.
Some tests aren't passing in Python 3 yet, presumably because using
traceback.format_exc()inside an exception's constructor doesn't work anymore.All the tests still pass in Python 2.7.