Don't use tabs for indentation about sherpa

[olaurino]

We are aware that there are issues related to the code not being always compliant with PEP8, and we are willing to make things better. Some issues, like the one you reported, may be more relevant than others.

In any case, it would probably be wise not to change most of the files in the codebase in order to enforce PEP8 at once, especially with automated tools and before we have migrated all the tests to GitHub as well.

What if we focused on a subset of PEP8 issues (e.g. mixing tabs and spaces) and then started enforcing PEP8 for new commits?

I am thinking of something along the line of what is suggested in this SO answer

[taldcroft]

I think the mixed tabs and spaces should be a high priority since that can actually lead to errors and makes editing difficult.

As for PEP8-storming the whole code base (using the term on SO), I understand the idea against this , especially for a code base with a long git history. But in this case the reasons given there don't quite apply:

• lots of merge-conflicts: there are only a handful of code PRs at the moment to cause conflicts
• break git blame: at this moment @olaurino has all the blame 😄
• make code review difficult: ?

So if the project is ever to do a PEP8-storm, now would be the time. Although it isn't a huge deal, there are plenty of people like me that have PEP8 enabled in their editors. This makes it a bit painful to work on PEP8-bad code because alot of code is red. In this case I'll generally turn off PEP8 checking, which then makes it hard to make new code be compliant. And for me, turning off PEP8 will also turn off my pyflake checking which is quite useful for catching syntax errors and mistyped variable names in advance.

Anyway, not a huge deal. Just for kicks I did this to see what it would look like:

autopep8 --max-line-length 100 --ignore=E24,E226 --in-place --recursive .

https://github.com/sherpa/sherpa/compare/master...taldcroft:autopep8?diff=split&name=autopep8

[olaurino]

@taldcroft I tried to merge your branch with the one @DougBurke is working on (see PR #17) and there are several conflicts already, as one may expect (although flake8 in my vim still complains about line lengths, just as it did before ;-) ).

Also, while git blame currently only sees my user, I still employ cleartool annotate in the original codebase. Of course, I can do that now because the mapping is still rather close for most of the files: the more Sherpa will live on GitHub, the more the mapping with the old codebase will be meaningless, but the more git blame will be meaningful.

We are willing to make the Sherpa code more PEP8-compliant, and it looks like we agree that there are priorities, so let's make sure we find the best way to apply such changes. Your insight on managing PEP8-compliant code and using tools like autopep8 is more than welcome in this transition.

[cdeil]

@olaurino – Agreed this shouldn't be done as long as there's large open PRs like #17.

For me the only real PEP8 issue when reading the Sherpa code is the 8-character tabs that are used in some Sherpa files, because my editor uses 4-character tabs and then the code looks mis-indented and is very hard to read and edit (see example screenshot above).

This could be fixed with

autopep8 --select E101,W191 --in-place --recursive .

E101 indentation contains mixed spaces and tabs
W191 indentation contains tabs

This is a safe transformation to apply without reviewing the (huge) diff, because it preserves the Python semantics. But it might still be useful to review the diff if there are parts that are not covered by tests and where there actually might be bugs due to the mis-indentation.

As I said, I don't care so much about other PEP8 stuff, but it might still be simpler to fix them in one go instead of incremental improvements over the coming months ...

[taldcroft]

I agree with @cdeil that the tabs fix should be a priority because it poses a real problem and can be fixed without too much fear of errors.

Does the tab-only fix that @cdeil proposed conflict with #17? That PR might take a little while to get merged. I would suspect that in any case rebasing #17 wouldn't be troublesome.

[olaurino]

Does the tab-only fix that @cdeil proposed conflict with #17?

It does in 3 files, although I suspect the conflicts could be easily resolved by a human.

That PR might take a little while to get merged. I would suspect that in any case rebasing #17 wouldn't be troublesome.

@DougBurke, any thoughts?

[DougBurke]

I am very close (ie by the end of the week, if not earlier) to finishing the first part of the documentation changes. I would prefer it if an automated process be run after we merge the doc changes, rather than make me go through and change something that was done by a manual process.

If it absolutely has to be done now, then please merge my existing changes first.

[DougBurke]

For reference, I've finished adding to PR #17 now.

[DougBurke]

Now that #17 has been merged, is it possible to get a branch with the autopep8 fixes so we can review?

Doug

[cdeil]

Here's what the diff for autopep8 --select E101,W191 --in-place --recursive . would look like:
f5a0592

[DougBurke]

I got down to sherpa/astro/ui/tests/test_ui.py and things looked good, but there's some changes in this file that aren't PEP8 related. For example

+logger = logging.getLogger("sherpa")

and

+logger.setLevel(logging.ERROR)

[cdeil]

The accidental change @DougBurke mentioned is here:
f5a0592#diff-76435341bc8e0aaf55046d857c2b7669R25

I probably forgot to run

git clean -fdx

before

autopep8 --select E101,W191 --in-place --recursive .
git checkout -- versioneer.py

Anyways, I didn't make a PR, this was just because I wanted to see what it looks like.
Such a big PEP8 change is probably something you or @olaurino should do yourself to be sure it's done right and no other changes come along for the ride.

[DougBurke]

@cdeil - thanks for the clarification, and for providing the code in the first place. I just wanted to make sure I knew what I was looking at.

[DougBurke]

My PR #26 also has the changes to sherpa/astro/ui/tests/test_ui.py that I mentioned above to @cdeil - see bcd8c39 . I am pretty sure I started out from master, so I don't understand what I've done to get this code.

[olaurino]

There are other cross-cut activities that are going on, and they are all related to the tests: the first one is @taldcroft's PR #10, and the other one is my work on travis, the new testing infrastructure, and the porting of the regression tests to git/github: these other activities also largely overlap with PR #10.

I would prefer to have the new testing infrastructure in place before running autopep8. I don't expect autopep8 to break anything, but the tool can be run at any time, while rebasing the other PRs requires human work. So, I would rather have the PEP8 review to be tested by the new test infrastructure than have the latter to be rebased on top of the PEP8 review.

Unfortunately the long weekend and an issue with CIAO prevented me from publishing the new test infrastructure on my fork, but I hope to get it done tomorrow.

[cdeil]

@DougBurke @olaurino – FYI, the weird "incorrect change" by autopep8 in sherpa/astropy/ui/tests/test_ui.py is a line ending character issue:

$ autopep8 --select E101,W191 --in-place sherpa/astro/ui/tests/test_ui.py
$ git diff
diff --git a/sherpa/astro/ui/tests/test_ui.py b/sherpa/astro/ui/tests/test_ui.py
index 99686b6..37c2343 100644
--- a/sherpa/astro/ui/tests/test_ui.py
+++ b/sherpa/astro/ui/tests/test_ui.py
@@ -21,7 +21,8 @@
 from sherpa.utils import SherpaTest, SherpaTestCase, needs_data
 import sherpa.astro.ui as ui
 import numpy
-import logging^Mlogger = logging.getLogger("sherpa")
+import logging
+logger = logging.getLogger("sherpa")

Probably this should be fixed in a PR separate from the autopep8 one.
I don't know how to find "all weird characters" in a codebase off the top of my hat to see if there's more, but let me know if it would be helpful if I search how that's done and make a PR.

[DougBurke]

Aha, thanks @cdeil. I remember seeing that one swim by when I made it (ie
git diff on the command line) but had forgotten about it by the time I
got to the web review/check.

On Tue, May 26, 2015 at 4:21 PM, Christoph Deil [email protected]
wrote:

@DougBurke https://github.com/DougBurke @olaurino
https://github.com/olaurino – FYI, the weird "incorrect change" by
autopep8 in sherpa/astropy/ui/tests/test_ui.py is a line ending character
issue
http://stackoverflow.com/questions/1822849/what-are-these-ms-that-keep-showing-up-in-my-files-in-emacs
:

$ autopep8 --select E101,W191 --in-place sherpa/astro/ui/tests/test_ui.py
$ git diff
diff --git a/sherpa/astro/ui/tests/test_ui.py b/sherpa/astro/ui/tests/test_ui.py
index 99686b6..37c2343 100644
--- a/sherpa/astro/ui/tests/test_ui.py
+++ b/sherpa/astro/ui/tests/test_ui.py
@@ -21,7 +21,8 @@
from sherpa.utils import SherpaTest, SherpaTestCase, needs_data
import sherpa.astro.ui as ui
import numpy
-import logging^Mlogger = logging.getLogger("sherpa")
+import logging
+logger = logging.getLogger("sherpa")

Probably this should be fixed in a PR separate from the autopep8 one.
I don't know how to find "all weird characters" in a codebase off the top
of my hat to see if there's more, but let me know if it would be helpful if
I search how that's done and make a PR.

—
Reply to this email directly or view it on GitHub
#9 (comment).

[DougBurke]

@olaurino - for me this work convinced me that we should not expect any problems from the autopep8 work, but I'm happy to wait for the testing code if it comes soon. One thing you could do is to run autopep8 on the config/setup code as you merge in the various PRs that deal with them, since there's a few changes there (mainly of the 'add a new-line to the end of the file' variety).

[cdeil]

One way to fix the line endings issue is (see http://stackoverflow.com/a/22521008/498873)

find . -type f -print0 | xargs -0 -n 1 -P 4 dos2unix -c mac

The diff is here: 27fed23
I think this might be a bug in how Github displays diffs, there's lines missing that are in the original file:
https://github.com/sherpa/sherpa/blob/master/sherpa/ui/utils.py#L240

I'm not 100% sure, but I think applying this change might also be a good idea:

find . -type f -print0 | xargs -0 -n 1 -P 4 dos2unix -c ascii

d016732

@olaurino - Just wanted to let you know for when the time comes to run autopep8, I think after these line ending fixes it should work without introducing issues.

[olaurino]

See PR #58. I guess it does not hurt to merge it, but it should not be terribly useful either.