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

style: tighten ruff lints #1557

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danieleades
Copy link
Contributor

  • tighten ruff linting
  • normalise doc comments and some formatting

note that this applies ruff 'preview' formatting as a one off. This is compatible with the default ruff formatting

Copy link

codecov bot commented Mar 17, 2024

Codecov Report

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

Project coverage is 97.36%. Comparing base (4831baf) to head (3a4f7f4).

Files Patch % Lines
copier/user_data.py 0.00% 1 Missing ⚠️
devtasks.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1557      +/-   ##
==========================================
+ Coverage   97.30%   97.36%   +0.06%     
==========================================
  Files          48       48              
  Lines        4594     4590       -4     
==========================================
- Hits         4470     4469       -1     
+ Misses        124      121       -3     
Flag Coverage Δ
unittests 97.36% <97.26%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

devtasks.py Show resolved Hide resolved
{
(src / "copier.yml"): (
"""\
build_file_tree({
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a manual change too? I would find it surprising if Black/Ruff concatenated again ({ together instead of adding newlines and indentation levels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both black and ruff do this in --preview mode. likely to move into non-preview eventually but black doesn't move fast

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, OK, thanks 🙂

Copy link
Contributor

@pawamoy pawamoy Mar 18, 2024

Choose a reason for hiding this comment

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

Just to be clear: did you manually run Ruff with preview mode enabled? If yes: is the preview mode enabled in our CI (pre-commit / poe lint / GitHub workflows / etc.)?

If it is not enabled, then I think we should revert all these style changes (like the ones in tests; the line splits and other fixes are good), and let Ruff format the code again later when such formatting is out of preview mode.

Otherwise, we could consider enabling preview mode, but I'll leave that decision to other @copier-org/maintainers.

Also, maybe I'm just too nit-picky! I'm just surprised by this new style because Black got us used to newlines, always. And I worry that not being in sync could trigger back and forth re-styling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Locally, I reverted the changes in tests, ran poe lint again, and it didn't style the code as what we see in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if you

  1. Clone this PR
  2. Run 'poe lint' without reverting any other changes
  3. Commit and push back to this PR

?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does nothing, and succeeds :) So one or the other style is actually not enforced, in which case I think it's preferable to keep the current style. Just my opinion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do. Will need to find some time to manually revert- hopefully tonight

Copy link
Member

Choose a reason for hiding this comment

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

FWIW I prefer to disable preview mode of formatters. Having in mind that it's just cosmetic most times, I prefer to stick to stable releases and checks.

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.

3 participants