Issue 6028046: issue329
Descriptionpython3 compatibility
Patch Set 1 #
Total comments: 51
Patch Set 2 : check all python versions #MessagesTotal messages: 3
http://codereview.appspot.com/6028046/diff/1/misc/test-translator.py File misc/test-translator.py (right): http://codereview.appspot.com/6028046/diff/1/misc/test-translator.py#newcode37 misc/test-translator.py:37: tasks.sort() "f in sorted(os.listdir..." + tasks.sort() looks redundant http://codereview.appspot.com/6028046/diff/1/src/translate/build_model.py File src/translate/build_model.py (left): http://codereview.appspot.com/6028046/diff/1/src/translate/build_model.py#old... src/translate/build_model.py:199: predicates.sort() These two lines could be more nicely combined to predicates = sorted(self.predicate_to_rule_generator) http://codereview.appspot.com/6028046/diff/1/src/translate/build_model.py File src/translate/build_model.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/build_model.py#new... src/translate/build_model.py:276: assert isinstance(self.__next__, MatchGenerator) I don't think this is correct. Is this tested? "next" here has nothing to do with Python's next/__next__. (And even if it did, self.__next__ would not work in Python 2.x.) http://codereview.appspot.com/6028046/diff/1/src/translate/build_model.py#new... src/translate/build_model.py:290: return self.__bool__() Better and more efficient: __nonzero__ = __bool__ http://codereview.appspot.com/6028046/diff/1/src/translate/fact_groups.py File src/translate/fact_groups.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/fact_groups.py#new... src/translate/fact_groups.py:52: return self.__bool__() See earlier comment. http://codereview.appspot.com/6028046/diff/1/src/translate/invariant_finder.py File src/translate/invariant_finder.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/invariant_finder.p... src/translate/invariant_finder.py:49: combs = itertools.combinations(list(range(len(action.parameters))), 2) This looks more than a little ugly. Is there no better way to do this? http://codereview.appspot.com/6028046/diff/1/src/translate/invariant_finder.p... src/translate/invariant_finder.py:80: all_args = list(range(len(predicate.arguments))) Ditto. http://codereview.appspot.com/6028046/diff/1/src/translate/invariants.py File src/translate/invariants.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/invariants.py#newc... src/translate/invariants.py:351: ensure_conjunction_sat(system, *itertools.chain(list(lhs_by_pred.values()))) Does chain not except a general iterable? http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/axioms.py File src/translate/pddl/axioms.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/axioms.py#new... src/translate/pddl/axioms.py:27: print("Axiom %s(%s)" % (self.name, ", ".join(args))) I don't think we need a list here; join should take an iterable. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py File src/translate/pddl/conditions.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py... src/translate/pddl/conditions.py:72: return self.hash < other.hash Is it enough to define __lt__? Why? It seems a bit dangerous to me to have __lt__ but not __le__, for example. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py... src/translate/pddl/conditions.py:128: __hash__ = Condition.__hash__ That doesn't seem to do anything useful? __hash__ would be inherited from the superclass anyway. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py... src/translate/pddl/conditions.py:156: __hash__ = Condition.__hash__ Ditto (and also for other cases below). http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/pddl_file.py File src/translate/pddl/pddl_file.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/pddl_file.py#... src/translate/pddl/pddl_file.py:20: def open_pddl_file(task_filename=None, domain_filename=None): Why the change? The way it was before seems more in the Python spirit. Compare from the Python library index: open() (built-in function) (FrameWork.DialogWindow method) (FrameWork.Window method) (distutils.text_file.TextFile method) (imaplib.IMAP4 method) (in module aifc) (in module anydbm) (in module cd) (in module codecs) (in module dbhash) (in module dbm) (in module dl) (in module dumbdbm) (in module gdbm) (in module gzip) (in module io) (in module os) (in module ossaudiodev) (in module posixfile) (in module shelve) (in module sunau) (in module sunaudiodev) (in module tarfile) (in module wave) (in module webbrowser) (pipes.Template method) (tarfile.TarFile method) (telnetlib.Telnet method) (urllib.URLopener method) (urllib2.OpenerDirector method) (webbrowser.controller method) (zipfile.ZipFile method) http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/pddl_types.py File src/translate/pddl/pddl_types.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/pddl_types.py... src/translate/pddl/pddl_types.py:54: from . import conditions Is there no better way to resolve this? http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/tasks.py File src/translate/pddl/tasks.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/tasks.py#newc... src/translate/pddl/tasks.py:102: assert next(iterator) == "define" This is a bug in the old code; the assertion shouldn't have a side effect that is important for correctness. Should be something like define_tag = next(iterator) assert define_tag == "define" Similarly for other places where "assert" and "next" are used together, I guess. http://codereview.appspot.com/6028046/diff/1/src/translate/sas_tasks.py File src/translate/sas_tasks.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/sas_tasks.py#newco... src/translate/sas_tasks.py:53: for var, (rang, axiom_layer) in enumerate(list(zip(self.ranges, self.axiom_layers))): I don't think we should convert to a list here. http://codereview.appspot.com/6028046/diff/1/src/translate/sas_tasks.py#newco... src/translate/sas_tasks.py:62: self.ranges, self.axiom_layers, self.value_names))): Ditto. http://codereview.appspot.com/6028046/diff/1/src/translate/simplify.py File src/translate/simplify.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/simplify.py#newcod... src/translate/simplify.py:235: (var_no, pre, post, cond) = xxx_todo_changeme xxx_todo_changeme => pre_post_tuple http://codereview.appspot.com/6028046/diff/1/src/translate/simplify.py#newcod... src/translate/simplify.py:257: (var_no, value) = xxx_todo_changeme1 xxx_todo_changeme1 => "fact_pair" or some such http://codereview.appspot.com/6028046/diff/1/src/translate/translate-relaxed.py File src/translate/translate-relaxed.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/translate-relaxed.... src/translate/translate-relaxed.py:70: eff_condition = list(eff_condition.items()) Do we do anything with eff_condition that needs a list? http://codereview.appspot.com/6028046/diff/1/src/translate/translate-relaxed.... src/translate/translate-relaxed.py:87: eff_condition = list(eff_condition_dict.items()) Same question. http://codereview.appspot.com/6028046/diff/1/src/translate/translate-relaxed.... src/translate/translate-relaxed.py:154: prevail = list(condition.items()) Same question. http://codereview.appspot.com/6028046/diff/1/src/translate/translate.py File src/translate/translate.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/translate.py#newco... src/translate/translate.py:574: task = pddl.open_pddl_file() Hmmm, in the old code pddl.open() was used in at least ten places, but it seems that only one of them was changed to pddl.open_pddl_file()? http://codereview.appspot.com/6028046/diff/1/src/translate/translate.py#newco... src/translate/translate.py:584: sas_task.output(open("output.sas", "w")) We should probably use "with" these days when opening files. But of course this is not really related to this issue.
Sign in to reply to this message.
I have incorporated your comments and will run an experiment now. http://codereview.appspot.com/6028046/diff/1/misc/test-translator.py File misc/test-translator.py (right): http://codereview.appspot.com/6028046/diff/1/misc/test-translator.py#newcode37 misc/test-translator.py:37: tasks.sort() On 2012/04/13 16:08:53, Malte wrote: > "f in sorted(os.listdir..." + tasks.sort() looks redundant Done. http://codereview.appspot.com/6028046/diff/1/src/translate/build_model.py File src/translate/build_model.py (left): http://codereview.appspot.com/6028046/diff/1/src/translate/build_model.py#old... src/translate/build_model.py:199: predicates.sort() On 2012/04/13 16:08:53, Malte wrote: > These two lines could be more nicely combined to > > predicates = sorted(self.predicate_to_rule_generator) Done. http://codereview.appspot.com/6028046/diff/1/src/translate/build_model.py File src/translate/build_model.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/build_model.py#new... src/translate/build_model.py:276: assert isinstance(self.__next__, MatchGenerator) This wasn't tested as it's only in a dump() function. Is there an easy way to make the translator more verbose and let it use those functions? http://codereview.appspot.com/6028046/diff/1/src/translate/build_model.py#new... src/translate/build_model.py:290: return self.__bool__() On 2012/04/13 16:08:53, Malte wrote: > Better and more efficient: > > __nonzero__ = __bool__ Done. http://codereview.appspot.com/6028046/diff/1/src/translate/fact_groups.py File src/translate/fact_groups.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/fact_groups.py#new... src/translate/fact_groups.py:52: return self.__bool__() On 2012/04/13 16:08:53, Malte wrote: > See earlier comment. Done. http://codereview.appspot.com/6028046/diff/1/src/translate/invariant_finder.py File src/translate/invariant_finder.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/invariant_finder.p... src/translate/invariant_finder.py:49: combs = itertools.combinations(list(range(len(action.parameters))), 2) The list() function is actually not needed here. http://codereview.appspot.com/6028046/diff/1/src/translate/invariant_finder.p... src/translate/invariant_finder.py:80: all_args = list(range(len(predicate.arguments))) On 2012/04/13 16:08:53, Malte wrote: > Ditto. Nicer: for omitted_arg in range(-1, len(predicate.arguments)): http://codereview.appspot.com/6028046/diff/1/src/translate/invariants.py File src/translate/invariants.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/invariants.py#newc... src/translate/invariants.py:351: ensure_conjunction_sat(system, *itertools.chain(list(lhs_by_pred.values()))) On 2012/04/13 16:08:53, Malte wrote: > Does chain not except a general iterable? You're right. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/axioms.py File src/translate/pddl/axioms.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/axioms.py#new... src/translate/pddl/axioms.py:27: print("Axiom %s(%s)" % (self.name, ", ".join(args))) On 2012/04/13 16:08:53, Malte wrote: > I don't think we need a list here; join should take an iterable. Done. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py File src/translate/pddl/conditions.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py... src/translate/pddl/conditions.py:72: return self.hash < other.hash On 2012/04/13 16:08:53, Malte wrote: > Is it enough to define __lt__? Why? I added the other ones to be on the safe side. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py... src/translate/pddl/conditions.py:128: __hash__ = Condition.__hash__ On 2012/04/13 16:08:53, Malte wrote: > That doesn't seem to do anything useful? > __hash__ would be inherited from the superclass anyway. I was surprised as well. From http://docs.python.org/release/3.2/reference/datamodel.html#object.__hash__: "If a class that overrides __eq__() needs to retain the implementation of __hash__() from a parent class, the interpreter must be told this explicitly by setting __hash__ = <ParentClass>.__hash__. Otherwise the inheritance of __hash__() will be blocked, just as if __hash__ had been explicitly set to None." http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py... src/translate/pddl/conditions.py:156: __hash__ = Condition.__hash__ On 2012/04/13 16:08:53, Malte wrote: > Ditto (and also for other cases below). Done. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/pddl_file.py File src/translate/pddl/pddl_file.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/pddl_file.py#... src/translate/pddl/pddl_file.py:20: def open_pddl_file(task_filename=None, domain_filename=None): This has been reverted. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/pddl_types.py File src/translate/pddl/pddl_types.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/pddl_types.py... src/translate/pddl/pddl_types.py:54: from . import conditions On 2012/04/13 16:08:53, Malte wrote: > Is there no better way to resolve this? I couldn't come up with a simple solution for this. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/tasks.py File src/translate/pddl/tasks.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/tasks.py#newc... src/translate/pddl/tasks.py:102: assert next(iterator) == "define" On 2012/04/13 16:08:53, Malte wrote: > This is a bug in the old code; the assertion shouldn't have a side effect that > is important for correctness. Should be something like > > define_tag = next(iterator) > assert define_tag == "define" > > Similarly for other places where "assert" and "next" are used together, I guess. Done. http://codereview.appspot.com/6028046/diff/1/src/translate/sas_tasks.py File src/translate/sas_tasks.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/sas_tasks.py#newco... src/translate/sas_tasks.py:53: for var, (rang, axiom_layer) in enumerate(list(zip(self.ranges, self.axiom_layers))): On 2012/04/13 16:08:53, Malte wrote: > I don't think we should convert to a list here. Done. http://codereview.appspot.com/6028046/diff/1/src/translate/sas_tasks.py#newco... src/translate/sas_tasks.py:62: self.ranges, self.axiom_layers, self.value_names))): On 2012/04/13 16:08:53, Malte wrote: > Ditto. Done. http://codereview.appspot.com/6028046/diff/1/src/translate/simplify.py File src/translate/simplify.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/simplify.py#newcod... src/translate/simplify.py:235: (var_no, pre, post, cond) = xxx_todo_changeme On 2012/04/13 16:08:53, Malte wrote: > xxx_todo_changeme => pre_post_tuple Done. http://codereview.appspot.com/6028046/diff/1/src/translate/simplify.py#newcod... src/translate/simplify.py:257: (var_no, value) = xxx_todo_changeme1 On 2012/04/13 16:08:53, Malte wrote: > xxx_todo_changeme1 => "fact_pair" or some such Done. http://codereview.appspot.com/6028046/diff/1/src/translate/translate-relaxed.py File src/translate/translate-relaxed.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/translate-relaxed.... src/translate/translate-relaxed.py:70: eff_condition = list(eff_condition.items()) On 2012/04/13 16:08:53, Malte wrote: > Do we do anything with eff_condition that needs a list? Yes, we need it as a list here: assert eff_condition in eff_conditions http://codereview.appspot.com/6028046/diff/1/src/translate/translate-relaxed.... src/translate/translate-relaxed.py:87: eff_condition = list(eff_condition_dict.items()) On 2012/04/13 16:08:53, Malte wrote: > Same question. Yes, we need it as a list here: assert eff_condition in eff_conditions http://codereview.appspot.com/6028046/diff/1/src/translate/translate-relaxed.... src/translate/translate-relaxed.py:154: prevail = list(condition.items()) On 2012/04/13 16:08:53, Malte wrote: > Same question. You're right list is not needed. http://codereview.appspot.com/6028046/diff/1/src/translate/translate.py File src/translate/translate.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/translate.py#newco... src/translate/translate.py:574: task = pddl.open_pddl_file() You're right. I undid the renaming. It had just been done, because the builtin open function was shadowed by the pddl.open function. http://codereview.appspot.com/6028046/diff/1/src/translate/translate.py#newco... src/translate/translate.py:584: sas_task.output(open("output.sas", "w")) On 2012/04/13 16:08:53, Malte wrote: > We should probably use "with" these days when opening files. But of course this > is not really related to this issue. Done.
Sign in to reply to this message.
A few more comments. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py File src/translate/pddl/conditions.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py... src/translate/pddl/conditions.py:72: return self.hash < other.hash On 2012/04/14 16:23:24, jendrik wrote: > On 2012/04/13 16:08:53, Malte wrote: > > Is it enough to define __lt__? Why? > I added the other ones to be on the safe side. Not sure which ones you added, but to be clear: it is in general necessary to define __eq__, __ne__, __lt__ and __le__ to get the full set of comparisons with all Pythons. __gt__ and __ge__ are automatically defined via __lt__ and __le__, but there is no automatic definition of (e.g). __le__ via __lt__ and __eq__ in any Python version. In Python 3.2 and possibly earlier versions of Python 3, __ne__ is also implemented by default if you have __eq__. But this is not true in Python 2.x. In summary: - For Python 2, we need __eq__, __ne__, __lt__ and __le__ (or __cmp__ instead of all those). - For Python 3, we need __eq__, __lt__ and __le__ There are currently discussions to define __le__ via __lt__ and __eq__ in the future, but from what I recall the proposal doesn't seem to be very positively received. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/conditions.py... src/translate/pddl/conditions.py:128: __hash__ = Condition.__hash__ On 2012/04/14 16:23:24, jendrik wrote: > On 2012/04/13 16:08:53, Malte wrote: > > That doesn't seem to do anything useful? > > __hash__ would be inherited from the superclass anyway. > > I was surprised as well. From > http://docs.python.org/release/3.2/reference/datamodel.html#object.__hash__: > "If a class that overrides __eq__() needs to retain the implementation of > __hash__() from a parent class, the interpreter must be told this explicitly by > setting __hash__ = <ParentClass>.__hash__. Otherwise the inheritance of > __hash__() will be blocked, just as if __hash__ had been explicitly set to > None." Aha! Can you change the comment a bit to reflect what is going on? Like # Defining __eq__ blocks inheritance of __hash__, so must set it explicitly. http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/pddl_types.py File src/translate/pddl/pddl_types.py (right): http://codereview.appspot.com/6028046/diff/1/src/translate/pddl/pddl_types.py... src/translate/pddl/pddl_types.py:54: from . import conditions On 2012/04/14 16:23:24, jendrik wrote: > On 2012/04/13 16:08:53, Malte wrote: > > Is there no better way to resolve this? > I couldn't come up with a simple solution for this. Can you add a # TODO: Try to resolve the cyclic import differently or something like this?
Sign in to reply to this message.
|
