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

Get flake8 running on the codebase #266

Closed
mdickinson opened this issue Feb 17, 2020 · 9 comments
Closed

Get flake8 running on the codebase #266

mdickinson opened this issue Feb 17, 2020 · 9 comments
Assignees

Comments

@mdickinson
Copy link
Member

For the 5.0 release, I'd like to get flake8 running on the codebase, in the same way as we're already doing for Traits.

At the last count, the number of individual complaints from flake8 was almost 3000:

mirzakhani:envisage mdickinson$ flake8 . | wc -l
    2825

So this looks like a daunting task.

However, I think we can approach this incrementally: start by adding a setup.cfg with a list of flake8 warnings and errors to ignore (and while we're at it, exclude the examples directory). Add warnings and errors to the ignore list until nothing is reported. Now remove the more serious warnings and errors from the list, one at a time, fixing up the code to comply as we go.

The most important errors and warnings are the ones that are likely to catch future bugs: syntax errors, unused names, undefined names, unused imports, and the like.

For spacing errors, I'd recommend that we do a one-time application of black to the codebase.

@mdickinson mdickinson added this to the Envisage 5.0.0 milestone Feb 17, 2020
@mdickinson
Copy link
Member Author

At the last count, the number of individual complaints from flake8 was almost 3000:

#267 reduces the number from 2825 to 863.

@mdickinson
Copy link
Member Author

See #268 for the machinery.

Just for fun, here's a breakdown of the current warnings on master:

mirzakhani:envisage mdickinson$ flake8 . --format="%(code)s" | sort | uniq -c | sort -r
 543 E266
 169 F401
  66 E501
  17 F821
  15 E722
  11 E231
   9 E401
   6 F841
   5 W605
   4 E203
   3 W601
   3 E713
   3 E402
   2 E741
   2 E302
   2 E251
   1 F403
   1 E731
   1 E721

@kitchoi
Copy link
Contributor

kitchoi commented Oct 21, 2020

So flake8 is already running on CI, I saw that there are some ignores in the setup.cfg.

Trying to understand what needs to happen in order to close this issue: I am not sure an empty setup.cfg is what we are shooting for here. I am not sure if examples is meant to stay there. */api.py:F401 probably should stay.

@rahulporuri
Copy link
Contributor

I think this issue can be closed @kitchoi . The examples which we will be including in envisage can be flake8-d once we move them into the package and envisage.ui.single_project has been deprecated and is meant to be removed - so there is no use flake8-ing it.

closing issue.

@mdickinson
Copy link
Member Author

Yes, sorry; this should have been closed when #268 was merged.

@kitchoi
Copy link
Contributor

kitchoi commented Oct 21, 2020

I see. Thank you.
I have started flaking envisage/ui/single_project a bit. I will try to push something meaningful for what I have got so far.

@mdickinson
Copy link
Member Author

I'm not sure it's worth it; unless there's a strong reason to keep it, I'd recommend that we just delete single_project.

@kitchoi
Copy link
Contributor

kitchoi commented Oct 21, 2020

Yeah as soon as I started looking into it, I found some dead code and issues about deleting it, but there are also some light uses of it. I just did what is cheap to do.

@rahulporuri
Copy link
Contributor

i've opened #327 to discuss removing envisage.ui.single_project.

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

No branches or pull requests

3 participants