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

Linting issues #22

Open
scottkleinman opened this issue Aug 8, 2018 · 2 comments
Open

Linting issues #22

scottkleinman opened this issue Aug 8, 2018 · 2 comments

Comments

@scottkleinman
Copy link
Contributor

scottkleinman commented Aug 8, 2018

As I gradually enable stricter linting rules, I'm using this issue to record specific points. Discussion in the comments is welcome.

wrong-import-order and wrong-import-position

These rules impose a requirement that imports like from app.projects.helpers import workspace appear at the top of the file and after import requests, which must itself come below other Python imports. Since these rules are contradictory, linting fails. It should be possible to disable one of these sub-requirements, but, until then, these rules have been left off.

Jeremy's helpful section headers helped solved this one.

bare-except

Enabling this rule would require a major re-factor since except statements play a variety of roles in the current code base, sometimes catching failures along chains of code. Satisfying the bare minimum with except Exception does not work because the rule considers this insufficiently specific. Applying the rule as intended would probably require a coder with substantial experience using Python exceptions and the time to re-design the code base along lines that, whilst clearly better, might not be possible given the constraints of an academic project. This rule has therefore been left disabled.

Pylintrc CLEAN LOGIC section

Passes all tests locally, but not on Travis.

@jeremydouglass
Copy link
Contributor

I took a quick look, and it looks like ~1/4 of the bare except statements come after a clause containing assert -- and their error text suggests that the exception to be caught is the corresponding AssertionError. I changed those as part of a first pass and pushed it to a bare-except branch to take a look (untested).

I agree that going through the rest would take significant time and maybe should not be high priority ahead of getting to alpha. We can just add clauses to each try as we go, I think -- no major refactor.

@jeremydouglass
Copy link
Contributor

Hey, Scott, you mentioned "CLEAN LOGIC" passes all tests locally, but not on Travis. Could you share some error log for that, or a link to the failing Travis build so I could check out the log? Is that all tests being active?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants