add flake8 pre-commit

[Zethson]

I intervened a couple of times manually and switched off a couple of checks, but it should be a strong improvement.

Signed-off-by: Zethson [email protected]

[Zethson]

E ImportError: cannot import name 'settings' from partially initialized module 'scanpy' (most likely due to a circular import) (/home/vsts/work/1/s/scanpy/__init__.py)

Meh, it's hell to track this down now. I assume that autopep8 removed an unused variable.

[Zethson]

$ python -m scanpy.tests.blackdiff 10

/home/travis/virtualenv/python3.7.1/lib/python3.7/site-packages/setuptools_scm/git.py:68: UserWarning: "/home/travis/build/theislab/scanpy" is shallow and may cause errors

  warnings.warn('"{}" is shallow and may cause errors'.format(wd.path))

/home/travis/virtualenv/python3.7.1/bin/python: No module named scanpy.tests.blackdiff

Not sure why this happened, but we well we're using black with pre-commit now anyways so w/e. Does this need fixing?

[ivirshup]

Main thing: Can you get rid of the line length change and re-request review?

---

scanpy.tests.blackdiff

This file got deleted, so I'm actually kinda surprised you're only getting a warning. Whatever this check is can be removed.

[ivirshup]

Left a fair amount of comments and definitely didn't get everything.

Commit process

I think this review process will be easier if you can limit changes from one flake error type to a single commit.

This will make it easier to:

• Evaluate rules on their own. Sometimes rules aren't totally clear. I'm quite confused by some of the times you've used # noqa: F821
• Lets me know what rule the line was violating
• Revert changes if we decide we want to deactivate a particular rule. There are a few instances of this already, and I'd like to minimize the pain of this process.

This may be kinda hard to do now, but does this sound like a reasonable way to go forward?

Rules I don't like

• I don't like replacing x == False with not x in all cases. Sometimes a variable could be a container, and an error should be thrown. I think cases have to be evaluated for this.
• Whats with changing from single letter variables inside expressions? Seems fine to me.
• `lambda's also are generally fine.
• Whats up with removing leading #s from comments?

Process for bugs

So, some of the things you've adding a # noqa to look like bugs. I think we need to have a plan in place for doing something about these. Do you have any suggestions?

[Zethson]

About the commit process: That's far far too much work to do it like you suggested. I don't have the time for this.

About the rules:

1. "I don't like replacing x == False with not x in all cases. Sometimes a variable could be a container, and an error should be thrown. I think cases have to be evaluated for this."

This should be covered by tests. In any case it is not good style and a violation.

2. "Whats with changing from single letter variables inside expressions? Seems fine to me."

They are redefinitions of earlier variables and trip up flake8. We can call them whatever we want as long it s not l again.

3. "`lambda's also are generally fine."

See comment at the section.

4. "Whats up with removing leading #s from comments?" Not my choice either. What we have now is pep8 and flake8 compliant. If you're not happy with this we can ignore the rule.

5. "So, some of the things you've adding a # noqa to look like bugs. I think we need to have a plan in place for doing something about these. Do you have any suggestions?"

The noqa ignore a rule for a specific line. I did not want to "fix" these things myself since Python is a dynamic language and you never know what happens :) Ideally we eventually get rid of all noqas, but not in this PR and not by me. I don't know the internals well enough to know whether this could have any side effects.

[Zethson]

@ivirshup tried to address all comments and made changes when necessary.

[Zethson]

@ivirshup I would keep the noqas. They are very easily searchable across the whole project and can be fixed later.

To me the PR is fine now. Any final wishes?

[ivirshup]

About the commit process: That's far far too much work to do it like you suggested. I don't have the time for this.

As a general point about this PR: to me, the fair amount of the work of turning on flake8 deciding on the rules. Perhaps we should start with a subset of files then? I realize you did not come to the meeting where we talked about this, so perhaps there is a difference of expectations here, but we agreed to be conservative about the rules we turned on in pre-commit.

Going through everything to make sure changes are correctly reverted is also takes a lot of time for me as the reviewer. I'd also like to limit that.

---

You said you used some automated tools to get faster compliance. What were these? In general, I would prefer to have a formatter that automatically ran than a tool that told me I formatted something wrong.

---

@ivirshup I would keep the noqas. They are very easily searchable across the whole project and can be fixed later.

I'm pretty strongly against this. noqas just look like the formatter/ linter was wrong, and I'm not accepting that having no plan to address bugs. I think this should be a discussion with a broader set of the team.

"Whats up with removing leading #s from comments?" Not my choice either. What we have now is pep8 and flake8 compliant. If you're not happy with this we can ignore the rule.

Yes, lets ignore this.

"I don't like replacing x == False with not x in all cases. Sometimes a variable could be a container, and an error should be thrown. I think cases have to be evaluated for this."

This should be covered by tests. In any case it is not good style and a violation.

I will try and take a closer look at these changes. I'm particularly concerned that there will be cases where possible values are None, True, and False.

[ivirshup]

Thanks for the changes, just a few minor things I've added suggestions for and the paga variables.

[ivirshup]

Looks good, thanks for all the work!

We should add a release note for this at some point, I'm just not sure where yet, probably a section for dev practices. Could you suggest a line for that?

I was unsure about the variable naming for PAGA, so I've decided to revert that. I couldn't get flake8 to call it a redefinition.

[Zethson]

Looks good, thanks for all the work!

We should add a release note for this at some point, I'm just not sure where yet, probably a section for dev practices. Could you suggest a line for that?

I was unsure about the variable naming for PAGA, so I've decided to revert that. I couldn't get flake8 to call it a redefinition.

🎉
Maybe "Enabled flake8 (https://flake8.pycqa.org/en/latest/) pre-commit to run code style checks"?
Everything else might just be details that people will uncover anyways since the workflows might complain :)