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

ci: Replace lint and formatting tools with ruff #1074

Merged
merged 38 commits into from
Mar 18, 2024

Conversation

mitches-got-glitches
Copy link
Contributor

@mitches-got-glitches mitches-got-glitches commented Mar 6, 2024

Context

I thought I would have a go at adding ruff to the codebase in anticipation of using it for #1060. My initial thought was to suggest pydocstyle for that ticket, but this project has been deprecated in favour of ruff. The speed benefits of using ruff are pretty significant:
image

Throughout doing this ticket, I have figured out that we could add ruff and tell it only to check the pydocstyle codes - so that is an alternative if this change is a bit much.

Key changes

Notes

Extras

  • The ignore rules in the ruff.toml can potentially be tuned as per-file-ignores
  • I can offer some extra proof that some of the things that have been removed still work with ruff - just let me know what you want to see.

Replaces black, flake8 and pyupgrade. The flake8 plugins are covered in
the ruff.toml.
This was being offered by the pre-commit hook which has just been
removed: reorder-imports...
This is closest to how things were before.
Only ignores print warnings now.
Not sure about this one since it treats dynaconf and pytest as part of
the same group, perhaps because it's not part of the package?
Doesn't need to be replaced with ruff since it will run as part of the
pre-commit section.
Copy link

netlify bot commented Mar 6, 2024

Deploy Preview for dynaconf ready!

Name Link
🔨 Latest commit 33013a5
🔍 Latest deploy log https://app.netlify.com/sites/dynaconf/deploys/65f87625b7d7c700081999a8
😎 Deploy Preview https://deploy-preview-1074--dynaconf.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 91.30435% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 98.75%. Comparing base (b29fc06) to head (33013a5).
Report is 1 commits behind head on master.

Files Patch % Lines
dynaconf/validator.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1074      +/-   ##
==========================================
- Coverage   98.80%   98.75%   -0.05%     
==========================================
  Files          23       23              
  Lines        2253     2248       -5     
==========================================
- Hits         2226     2220       -6     
- Misses         27       28       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@pedro-psb pedro-psb left a comment

Choose a reason for hiding this comment

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

That looks good. I can see it catches TODO, breakpoints and prints, and seems to do a good job at sorting the imports. Also, I don't mind the known style deviations.

Can you hard-comment the reason for tests/ruff.toml and 'test_functional/ruff.toml` ? I think it can be helpful.

dynaconf/cli.py Show resolved Hide resolved
Makefile Show resolved Hide resolved
ruff.toml Show resolved Hide resolved
ruff.toml Outdated Show resolved Hide resolved
dynaconf/utils/inspect.py Show resolved Hide resolved
dynaconf/hooking.py Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
requirements_dev.txt Outdated Show resolved Hide resolved
@pedro-psb
Copy link
Member

We should wait @rochacbruno review as well, as these are pretty significant changes.

And out of curiosity, why would we want per-file-ignores? I'm not saying we don't, I just don't know.

@mitches-got-glitches
Copy link
Contributor Author

We should wait @rochacbruno review as well, as these are pretty significant changes.

And out of curiosity, why would we want per-file-ignores? I'm not saying we don't, I just don't know.

Just as an example, if I enable a check on E402: Module level import not at the top of the file, we end up with these issues reported back:
image

Now we might not care too much about the rule contraventions in tests_functional/ but maybe we do care about it in dyanconf/cli.py. So we could have something like this in the ruff.toml:

[lint.per-file-ignores]
"tests_functional/*" = ["E402"]

And we end up with this:
image

The checks that we currently ignore may be worth applying over the dynaconf/ folder at least. What do you think?

Everything can now be controlled from one file. And it resolves
potential issues by having the whole "lint" section overwritten. For
example, by changing this it picked up that the imports weren't being
sorted in one file, since this section was overridden in the lower
config file.
It's already enforced by urff under the [lint.isort] settings
Meant that line-by-line blanket # noqa pragmas could be removed.
@mitches-got-glitches
Copy link
Contributor Author

Can you hard-comment the reason for tests/ruff.toml and 'test_functional/ruff.toml` ? I think it can be helpful.

I've added some new commits which actually remove these in favour of per-file-ignores. Let me know what you think of the latest commits where I've implemented per-file-ignores. Tried to do everything on separate commits so things can be rolled back if needed.

Copy link
Member

@pedro-psb pedro-psb left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying per-ignore-files.
Its a nice feature and I prefer it over the extra config files.
This looks good for me! 🎉

@pedro-psb pedro-psb merged commit f2f8f2c into dynaconf:master Mar 18, 2024
46 of 47 checks passed
pedro-psb added a commit that referenced this pull request Mar 18, 2024
Shortlog of commits since last release:

    Aaron DeVore (1):
          docs: fix incorrect combination of TOML table and inline table (#1070)

    Adam Kjems (1):
          doc: replace dead link to flask subclassing page (#1031)

    Bruno Rocha (2):
          feat: Add `@get` converter to alias existing keys (#1040)
          fix: dependabot alert 21 about Django (on tests) (#1067)

    HAMASHITA (1):
          chore: Fix misspelled variable name (#1032)

    Lucas Limeira (1):
          doc: Add explicit Dynaconf instantiation to sample code (#1022)

    Mitchell Edmunds (5):
          docs: Add dynaconf API to docs with mkdocstrings (#1058)
          docs: Fix mkdocs warnings for cleaner build output (#1061)
          chore: add "typos" tool and run it in codebase/docs (#1063)
          Fix referencing issues in the docs (#1062)
          chore(ci): Replace lint and formatting tools with ruff (#1074)

    Mostafa Alayesh (1):
          doc: fix argument `env` in Validation at validation.md (#1051)

    Pedro Brochado (11):
          chore(ci): Fix misspelled GitHub action names on main.yml (#1033)
          chore(ci): move release workflow to GitHub actions (partial) (#1043)
          chore(ci): fix shell script in release.yml
          chore(ci): fix shell script in release.yml (2)
          chore(ci): fix create-release-commit.sh permission
          fix: include load with relative root path (#1049)
          fix: `_bypass_evaluation` showing in end-user settings (#1071)
          chore: Replace/Update release script (#1078)
          docs: fix wrong info about validation trigger on insantiation (#1076)
          misc: fix bump-my-version invalid config and rename bump msg
          misc: fix changelog generation order

    Shanshi Shi (1):
          Fix a typo in the docs about `merge_enabled` setting (#1044)

    Sun Jianjiao (1):
          doc: Fix the syntax errors in the sample program. (#1027)

    tdzz1102 (1):
          [Doc] Fix a small mistake of .env file in the document (#1036)

    xiaohuanshu (1):
          doc: fix click help syntax error (#1041)
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.

None yet

3 participants