New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-14001: Make afw pep8 compliant and enable auto testing #341
Conversation
srcWcs = srcWcs, | ||
srcCtrInd = srcCtrInd, | ||
projName="TAN", | ||
destCtrInd=destCtrInd, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the original version was "correct": https://developer.lsst.io/python/style.html#keyword-assignment-operators-should-be-surrounded-by-a-space-when-statements-appear-on-multiple-lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the original version did not pass flake8 linter using our standard published flake8 configuration, which had fewer things disabled than my own settings. Sigh. I wish our standard was at least optional, but as it stands there is no simple way to reconcile our published list of flake8 ignores and our style guide. I filed RFC-471
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really do think we should use PEP8 here. I don't understand the motivation for the space. It seems to me that if you have a longish line, and then add a new keyword and have to go on a new line, that you suddenly need to add spaces everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look great to me.
- python: '3.6' | ||
install: | ||
- pip install flake8 | ||
script: flake8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does my example also lack the new line?
@@ -32,8 +32,7 @@ | |||
import argparse | |||
|
|||
parser = argparse.ArgumentParser(description='Show the layout of CCDs in a camera.', | |||
epilog= | |||
'The corresponding obs-package must be setup (e.g. obs_decam ' + | |||
epilog='The corresponding obs-package must be setup (e.g. obs_decam ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the +
needed? (I know you didn't add it).
@@ -408,7 +410,7 @@ def writeRGB(fileName, rgbImage): | |||
# | |||
|
|||
|
|||
class asinhMappingF(object): | |||
class asinhMappingF(object): # noqa N801 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is so deprecated that we no longer need it...
"key order is transform order", | ||
keytype = int, | ||
itemtype = OneTransformConfig, | ||
doc="Dict of index: OneTransformConfig (a transform wrapper); " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+
may not be necessary.
e1 = geom.Extent3D(1.2, 3.4, 5.6) | ||
|
||
e1 = geom.Extent3D(1.2, 3.4, 5.6) | ||
with self.assertRaises(TypeError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this improvement.
@@ -58,7 +58,10 @@ def checkAstropy(image, filename, hduNum=0): | |||
""" | |||
print("Astropy currently doesn't read our compressed images perfectly.") | |||
return | |||
parseVersion = lambda version: tuple(int(vv) for vv in np.array(version.split("."))) | |||
|
|||
def parseVersion(version): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much clearer. Thank you.
A trailing space is not needed when concatenating two string constants.
testGetNonContiguous in test_sourceTable.py used self.assertFloatsEqual to compare two bool arrays; use np.testing.assert_equal instead.
We long ago moved all Doxygen documentation out of .cc files into .h files. Stop scanning the .cc files, as it slows down Doxygen and is a frequent source of warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with the doxygen warning fixes being combined with the pep8 fixes in this PR.
No description provided.