Skip to content
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

Adopt flake8 #1908

Merged
merged 26 commits into from
Jun 21, 2022
Merged

Adopt flake8 #1908

merged 26 commits into from
Jun 21, 2022

Conversation

thorstenhater
Copy link
Contributor

This builds on top of the previous PRs regarding Python best practices and add the
flake8 linter.

@brenthuisman
Copy link
Contributor

brenthuisman commented Jun 16, 2022

flake8: good idea, but! readme says it's very tied to a Python minor version. You anointed Python 3.9 as standard, but since we want to keep our code compatible down to 3.6 (for now), should then the Action not run with 3.6?

Or is this more hassle than it's worth, considering the limited amount of Python?

Also, merge master pls.

@thorstenhater
Copy link
Contributor Author

  • The version thing I had read, but the annotations flake8 came back with where fine and independent of the version
  • We can go to 3.6 by that same argument
  • It found at least one stupid bug, so worth it. And I expect our Python fundus to be growing.
# gen-labels.py
def write_morphology(name, morph):
    string = "tmp = [".format(name)
    for i in range(morph.num_branches):

@brenthuisman
Copy link
Contributor

Let's go with the sanest choice wrt versions would be to run flake8 at the minimum version we support, 3.6 atm.

Copy link
Contributor

@brenthuisman brenthuisman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's left are the line numbers in the tutorials. I could take care of it in a follow up too, there's a few more tutorial mistakes left.

# artifacts
build,
# nah, don't care
spack/package.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, they care! They have the same check running.

@thorstenhater
Copy link
Contributor Author

Line numbers to be fixed after all the convenience, Python formatting, and linting PR are in.

@brenthuisman brenthuisman merged commit f685f0e into arbor-sim:master Jun 21, 2022
@thorstenhater thorstenhater deleted the python/flake8 branch August 1, 2022 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants