PEP8 and coverage increase #16224
PEP8 and coverage increase #16224
Conversation
|
✅ Hi, I am the SymPy bot (v142). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.
Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it. Click here to see the pull request description that was parsed. |
| @@ -232,7 +232,8 @@ def __cmp__(self, other): | |||
| return (a > b) - (a < b) | |||
|
|
|||
| def __str__(self): | |||
| return '%s(%s)' % (self.__class__.__name__, ', '.join(str(a) for a in self.args)) | |||
| return '%s(%s)' % (self.__class__.__name__, | |||
| ', '.join(str(a) for a in self.args)) | |||
There was a problem hiding this comment.
Here (and below) I don't prefer to have these large hanging indents. IMO, it doesn't add that much to legibility (since I still have to read it sequentially) and it creates more lines when there are longer lists of args. That was part of the rationale for filldedent which allows one to not have to worry about where the text is indented to:
...
filldedent('''
This is the text of the long message
that needs to be printed.''')
...
(Although last I recall, some wanted this indented properly which means you have lots of short lines...but filldedent makes it look fine in the end.)
There was a problem hiding this comment.
In this particular case the line broke the 80 character limit.
I can only agree that it is sometimes an issue when one tries to avoid long lines and the code is having several levels of indentation already.
So your suggestion is to sometimes break the 80 character limit?
I think that in this case both 80+ and the hanging indent are quite OK, but there are clearly those that look worse below...
I didn't see this comment before I pushed the commit below. Will look further into it tomorrow.
There was a problem hiding this comment.
I find it a bit hard to know when to format how. Would it be OK to merge this (and #16223) as is and I promise I will not change anything like this in future PRs? (It may happen that I format new code like it, based on the inability to judge so it is easier to follow rules, but no more PRs like this and #16223 where the main purpose is to reformat, following PEP8.)
There was a problem hiding this comment.
So your suggestion is to sometimes break the 80 character limit?
No, I mean I don't like the "intelligent" hanging indent. I prefer just
return '%s(%s)' % (self.__class__.__name__,
', '.join(str(a) for a in self.args))
There was a problem hiding this comment.
And I try to follow using 8 spaces if the next line is going to be indented:
if (a long
condition):
do this
There was a problem hiding this comment.
OK! I was confused since the example was for an 80+ character line. But there are clearly examples of those hanging indents elsewhere.
There was a problem hiding this comment.
The 80 character limit is one of the softer rules in PEP 8. Actually it says:
Some teams strongly prefer a longer line length. For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the line length limit up to 99 characters, provided that comments and docstrings are still wrapped at 72 characters.
I personally avoid wrapping long lines in cases where there isn't an obvious place to break the line (this isn't an example of that, as breaking after a comma is usually fine).
I also don't like the "hanging indent" rule either, as it tends to exacerbate the issue by indenting more than necessary, forcing even more line breaks.
The fact that we don't have immediate agreement on these things is why we don't have PEP 8 code quality checks in SymPy (beyond the ones we can agree on like trailing whitespace).
sympy/logic/tests/test_boolalg.py
Outdated
| @@ -932,6 +932,7 @@ def test_relational_simplification(): | |||
| w, x, y, z = symbols('w x y z', real=True) | |||
| d, e = symbols('d e', real=False) | |||
| assert Or(x >= y, x < y).simplify() == S.true | |||
| assert Or(y <= y, x < y).simplify() == S.true | |||
There was a problem hiding this comment.
This should have been y <= x... But there are more things to fix.
References to other Issues or PRs
Brief description of what is fixed or changed
Increase code quality based on PEP8
Increase coverage in sympy.logic.boolalg
Other comments
Release Notes
NO ENTRY