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

feat: add incremental linting and formatting #1328

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

feat: add incremental linting and formatting #1328

wants to merge 3 commits into from

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Oct 25, 2023

fixes #962

Summary:

  • Add inv lint which uses darker and lint-diffs to format and lint the changes on a given branch (by default relative to origin/dev).
  • Add CI action that will run inv lint on PRs.

Prerequisites:

  • Unless it is a documentation hotfix it should be merged against the dev branch
  • Branch is up-to-date with the branch to be merged with, i.e. dev
  • Build is successful
  • Code is cleaned up and formatted

Copy link
Collaborator

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

cool stuff! do you have a sample output?

tasks.py Outdated Show resolved Hide resolved
tasks.py Outdated Show resolved Hide resolved
pyproject.toml Outdated
Comment on lines 17 to 34
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_]*'
Copy link
Collaborator

Choose a reason for hiding this comment

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

i reserve the right to complain about these (or lack of them) as we use this more!

Copy link
Contributor Author

@dshemetov dshemetov Nov 2, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@dshemetov
Copy link
Contributor Author

dshemetov commented May 15, 2024

@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 dshemetov changed the title feat: add incremental linting feat: add incremental linting and formatting May 15, 2024
@dshemetov dshemetov requested a review from melange396 May 24, 2024 23:12
* add `inv lint` command to tasks.py
* add `lint` workflow to .github/workflows/lint.yaml
* update README.md with linting instructions
Copy link

sonarcloud bot commented Jun 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@cmu-delphi cmu-delphi deleted a comment from sonarcloud bot Jun 7, 2024
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.

Design and enforce linting standards
2 participants