Closed jyn514 closed 6 years ago
Weird, travis logs aren't showing up for some reason. I did hack around with /dev/tty, I guess that could be related? Not sure.
@jyn514
Proceed with the merge after considering my comments. No style or functionality concerns really.
Very nice work. I, glad to see pylint compliance at this level so quickly.
The shelldoc part is pretty trivial. I’m honestly not sure why I went with that instead of a python docstring. We should use docstrings for the python code since that’s the native standard.
Of course any of the test scripts that use shelldoc will continue using it since bash does not have its own format.
On Mar 2, 2018, at 08:58, Joshua Nelson notifications@github.com wrote:
@jyn514 commented on this pull request.
In bitshuffle/bitshuffle.py:
@@ -1,5 +1,7 @@
!/usr/bin/env python
+# pylint: disable=missing-docstring Pylint doesn't recognize your SHELLDOC format, I thought that would serve as a suitable docstring
— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.
Note that the pylint enforcement currently fails with a syntax error, probably made a local change and forgot to commit. Will fix once I'm not on mobile
Fixed; forgot to install on travis
This looks like a lot, but it doesn't change any logic or features. All unit tests are passing and I feel pretty comfortable. The largest changes, 337-398, are a badly indented block; there are few major changes.
The errors for pylint are fairly lenient and should be easy to avoid as long as we take a modicum of care. I set the error codes to increase by 1 for every point missed out of 10. When I opened the branch it was around 6; it's currently at 9.85. I also pylint to show warnings on commit; I think they are appropriately verbose.
Current messages on commit:
This closes https://github.com/charlesdaniels/bitshuffle/issues/53.