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: unify linter settings across indicators and add incremental formatting #1905

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Oct 25, 2023

Description

Related to cmu-delphi/delphi-epidata#1328, unifying linting settings across indicators and these two repos.

Changelog

  • Use a single pyproject.toml for all indicators and delphi_utils, removed all .pylintrc
    • Made sure all indicators pass with the new config
    • Inlined a few pylint ignores that were previously covered by a rule in a .pylintrc, instead of turning them into global rules
    • Some whitespace formatting in the process (on the files I touched)
    • The line-length max is now bumped up to 120 from 100
    • Sorted the dependencies in the setup.py files of each indicator while I was at it (ignored in git blame)
  • Add make format command to apply formatting to the changes on your current branch
  • Update the template files as well
  • Add a new darker format job to CI
  • Add python package caching to CI
  • Add "Lint and Format" section to README

Copy link
Contributor

@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.

@dshemetov dshemetov force-pushed the ds/lint branch 5 times, most recently from b0d1c25 to 8e5a2b0 Compare May 15, 2024 20:27
@dshemetov dshemetov changed the title feat: unify linter settings across indicators feat: unify linter settings across indicators and add incremental formatting May 15, 2024
@dshemetov dshemetov force-pushed the ds/lint branch 2 times, most recently from c24e3b0 to 46eb589 Compare June 5, 2024 18:56
* all linters now use the same configuration file
* add `make format` command to format all code
* make sure all indicators pass with the new config
* update the template files as well
* add a new darker format job to CI
* add python package caching
* update README
@dshemetov
Copy link
Contributor Author

This was approved a while ago, I'm just going to merge so the new pipeline code can benefit from these changes.

@dshemetov dshemetov merged commit a6ea003 into main Jun 5, 2024
15 checks passed
@dshemetov dshemetov deleted the ds/lint branch June 5, 2024 19:48
@melange396
Copy link
Contributor

looks like there were a lot of recent (force-pushed) changes since the old review... can you summarize them?

@dshemetov
Copy link
Contributor Author

dshemetov commented Jun 6, 2024

Sure, I just

  • updated the darker pin from darker[flynt]~=1.7.1 to darker[isort]~=2.1.1 (we don't use the flynt functionality and isort was listed as a dependency in a separate line, so this bundles them)
  • updated the templates to contain make format and have the darker dependency
  • update the README file with some linting and formatting instructions
  • updated the python CI so it runs the darker lint (couldn't get it to work before)
  • added Python package caching to CI

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.

2 participants