feat: add incremental linting and formatting #1328
Conversation
dshemetov
force-pushed
the
ds/lint
branch
2 times, most recently
from
00d74ef to
f5408cc
Compare
pyproject.toml
Outdated
| disable = [ | ||
| 'logging-format-interpolation', | ||
| # Allow pytest functions to be part of a class | ||
| 'no-self-use', | ||
| 'too-many-locals', | ||
| 'too-many-arguments', | ||
| 'too-many-branches', | ||
| # Allow pytest classes to have one test | ||
| 'too-few-public-methods', | ||
| ] | ||
| enable = 'useless-suppression' | ||
|
|
||
| [tool.pylint.'BASIC'] | ||
| # Allow arbitrarily short-named variables. | ||
| variable-rgx = ['[a-z_][a-z0-9_]*'] | ||
| argument-rgx = [ '[a-z_][a-z0-9_]*' ] | ||
| attr-rgx = ['[a-z_][a-z0-9_]*'] | ||
| [tool.pylint.basic] | ||
| # Allow arbitrarily short-named variables. | ||
| variable-rgx = '[A-Za-z_][a-z0-9_]*' | ||
| argument-rgx = '[A-Za-z_][a-z0-9_]*' | ||
| attr-rgx = '[A-Za-z_][a-z0-9_]*' |
There was a problem hiding this comment.
i reserve the right to complain about these (or lack of them) as we use this more!
There was a problem hiding this comment.
Fair! Tbh, as per the comment, I think the main reason for this is we had some mathy code that used mathy variables like X or W for matrix operations (which I think is reasonable) and the linter complained. So we wound up with this essentially YOLO arbitrary names regex.
A possible alternative solution is to upgrade to Pylint 3.0.0:
The invalid-name message no longer checks for a minimum length of 3 characters by default. (This was an unadvertised commingling of concerns between casing and name length, and users regularly reported this to be surprising.)
I could try bumping pylint in covidcast-indicators and if 3.0.0 passes there, we could probably then pin in both repos. Definitely don't want to satisfy different linter versions / settings across repos.
There was a problem hiding this comment.
So upgrading pylint in covidcast-indicators produces a lot of new errors that will take a while to fix. I think we should just keep both delphi-epidata and covidcast-indicators at pylint==2.8.3 for now, since that will allow us to bring this repo up incrementally and that will already be a nice improvement. After a bit of that, we can think about upgrading pylint and addressing the new issues. We can also tighten up these naming conventions in a PR that addresses just naming.
|
@melange396 here's a sample output (many months later 😮💨). You can see the same output in the CI below. (venv) ➜ delphi-epidata git:(ds/lint) ✗ inv lint
--- src/client/delphi_epidata.py 2024-05-15 21:34:24.871474 +0000
+++ src/client/delphi_epidata.py 2024-05-15 21:50:15.409996 +0000
@@ -77,11 +77,13 @@
@retry(reraise=True, stop=stop_after_attempt(2))
def _request_with_retry(endpoint, params={}):
"""Make request with a retry if an exception is thrown."""
request_url = f"{Epidata.BASE_URL}/{endpoint}/"
if Epidata.debug:
- Epidata.logger.info("Sending GET request", url=request_url, params=params, headers=_HEADERS, auth=Epidata.auth) # add a little comment
+ Epidata.logger.info(
+ "Sending GET request", url=request_url, params=params, headers=_HEADERS, auth=Epidata.auth
+ ) # add a little comment
if Epidata.sandbox:
resp = requests.Response()
resp._content = b'true'
return resp
start_time = time.time()
src/client/delphi_epidata.py:82:0: C0301: Line too long (146/120) (line-too-long)
src/client/delphi_epidata.py:316:0: C0325: Unnecessary parens after 'not' keyword (superfluous-parens)
tasks.py:45:0: C0304: Final newline missing (missing-final-newline)
=== pylint: mine=3, always=0 |
dshemetov
changed the title
* add `inv lint` command to tasks.py * add `lint` workflow to .github/workflows/lint.yaml * update README.md with linting instructions
|
fixes #962
Summary:
inv lintwhich uses darker and lint-diffs to format and lint the changes on a given branch (by default relative to origin/dev).inv linton PRs.Prerequisites:
devbranchdev