Opened 5 years ago
Closed 5 years ago
#22232 closed enhancement (fixed)
py3 cleanup of symmetric_reduction.pyx
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  python3  Keywords:  
Cc:  tscrim, jdemeyer, aapitzsch  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  e88d4c7 (Commits, GitHub, GitLab)  Commit:  e88d4c72615aa6abe694f9ea7cf3e520cfcbd839 
Dependencies:  Stopgaps: 
Description
remove all call to cmp()
plus some pep8 changes
Change History (8)
comment:1 Changed 5 years ago by
 Branch set to u/chapoton/22232
 Cc tscrim jdemeyer aapitzsch added
 Commit set to a93e1cef43252ddc7d1bfa103675a9fadbace599
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
I disagree with this change:
 self._lm = [R(x) for x in self._lm] # I have no idea why  but it seems needed. + self._lm = [R(x) for x in self._lm] + # I have no idea why  but it seems needed
as it breaks the association of the comment with the line in question.
I also (still) disagree with returning NotImplemented
when the classes do not match.
Could we also remove some of the useless parentheses, such as while (p._p != 0):
?
comment:3 Changed 5 years ago by
 Commit changed from a93e1cef43252ddc7d1bfa103675a9fadbace599 to acffce79a8374dbc51bfc245c4d6fe98fb69677f
Branch pushed to git repo; I updated commit sha1. New commits:
acffce7  trac 22232 some details

comment:4 Changed 5 years ago by
Just done the trivial changes.
About the NotImplemented?, why do you disagree ? Python will know what to do with this answer, in principle. This is not an Error, but probably behaves more somehow like an "unknown" value.
You may have already explained, but I still see no other reasonable possibility.
comment:5 Changed 5 years ago by
We do know that they are not equal or comparable when the types do not match. It's not like there is ambiguity with, e.g., symbolics.
comment:6 Changed 5 years ago by
 Commit changed from acffce79a8374dbc51bfc245c4d6fe98fb69677f to e88d4c72615aa6abe694f9ea7cf3e520cfcbd839
Branch pushed to git repo; I updated commit sha1. New commits:
e88d4c7  trac 22232 detail

comment:7 Changed 5 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
That is acceptable to me.
comment:8 Changed 5 years ago by
 Branch changed from u/chapoton/22232 to e88d4c72615aa6abe694f9ea7cf3e520cfcbd839
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
python3 and pep8 cleanup of symmetric_reduction.pyx