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

Make linting useful #1930

Closed
clbarnes opened this issue Oct 11, 2019 · 6 comments
Closed

Make linting useful #1930

clbarnes opened this issue Oct 11, 2019 · 6 comments

Comments

@clbarnes
Copy link
Contributor

csslint is dead; stylelint seems to be today's preferred option. I don't think we get any value out of css linting anyway - it's just 3000 terminal lines (many more lines in the browser) to scroll past on travis. It may have been me who put it in there in the first place. Sorry about that.

@aschampion
Copy link
Contributor

I added csslint (I think during the stack viewer rewrite) because it did catch and lead to fixes for a lot out-of-spec and broken rules that were causing problems when styling new elements, especially around z-fighting. But I agree the version on travis had become noise because it couldn't be configured in a way meaningful for us.

@clbarnes
Copy link
Contributor Author

clbarnes commented Oct 14, 2019

I suppose this speaks to a more broad issue with our linting (flake8 especially) - we're not really getting any value out of it while we're accepting so many failures. Even if we ignored some of the more opinionated formatting rules which would be lengthy and disruptive to the git history to fix (e.g. 1500 failures for E128 continuation line under-indented for visual indent and 1300 for E231 missing whitespace after ','), a few hours spent getting it to a state where the lint output is actually meaningful and worth failing a build for could save us merging errors like those fixed in #1933 and #1928.

@clbarnes clbarnes changed the title Remove/replace CSSlint Make Linting Useful Again Oct 14, 2019
@clbarnes clbarnes changed the title Make Linting Useful Again Make linting useful Oct 14, 2019
@clbarnes
Copy link
Contributor Author

@pgunn is clearly thinking the same, given today's PRs... I have a branch with a more selective flake8 config and am working through some of the low-hanging fruit, let's join forces?

@clbarnes
Copy link
Contributor Author

clbarnes commented Oct 15, 2019

We're nearly there on a stable config/ quality in #1935 , but there are still a lot of ignored rules which it would be nicer to fix. The remaining rules are:

516   E126 continuation line over-indented for hanging indent
74    E127 continuation line over-indented for visual indent
1479  E128 continuation line under-indented for visual indent
277   E201 whitespace after '{'
271   E202 whitespace before '}'
55    E225 missing whitespace around operator
72    E226 missing whitespace around arithmetic operator
1316  E231 missing whitespace after ','
85    E241 multiple spaces after ','
275   E251 unexpected spaces around keyword / parameter equals
82    E252 missing whitespace around parameter equals
146   E261 at least two spaces before inline comment
519   E302 expected 2 blank lines, found 1
196   E303 too many blank lines (2)
17    E305 expected 2 blank lines after class or function definition, found 0
25    E402 module level import not at top of file
226   E501 line too long (135 > 120 characters)
159   E502 the backslash is redundant between brackets
16    E722 do not use bare 'except'
37    E731 do not assign a lambda expression, use a def
229   F401 'typing.Dict' imported but unused
3     F403 'from catmaid.models import *' used; unable to detect undefined names
32    F405 'abs_catmaid_path' may be undefined, or defined from star imports: configuration
146   F841 local variable 'ClientDatastore' is assigned to but never used
7     W503 line break before binary operator
74    W504 line break after binary operator

IMO priorities should be

  1. F403 and F405 star imports
  2. F401 unused imports
  3. F841 unused variables

E731 "don't assign lambdas" may help with mypy. E722 "bare excepts" could at least be except Exception to prevent them handling other interrupts.

@pgunn
Copy link
Contributor

pgunn commented Oct 15, 2019

Should I do PRs to your fork here or do you want to open it up to me as a contributor?

@clbarnes
Copy link
Contributor Author

I should have pushed my branch to the main repo rather than my fork, sorry. Make PRs to my fork for now, hopefully we can get the basic stuff merged soon, which will make it easier.

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

4 participants