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

Adopt ruff formatter instead of black #1051

Merged
merged 6 commits into from
Nov 9, 2023
Merged

Conversation

Luca-Blight
Copy link
Contributor

@Luca-Blight Luca-Blight commented Oct 31, 2023

Change Summary

[".github/workflows/ci.yml","Makefile", "pyproject.toml", "tests/requirements-linting.txt"]

Related issue number

closes #1048

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

@Luca-Blight Luca-Blight changed the title Adopt ruff formatter instead of black Adopt ruff formatter instead of black WIP Oct 31, 2023
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #1051 (558fdf8) into main (cb47b96) will decrease coverage by 3.67%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1051      +/-   ##
==========================================
- Coverage   93.26%   89.59%   -3.67%     
==========================================
  Files         104      106       +2     
  Lines       15814    16796     +982     
  Branches       35       35              
==========================================
+ Hits        14749    15049     +300     
- Misses       1058     1740     +682     
  Partials        7        7              

see 8 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb47b96...558fdf8. Read the comment docs.

Copy link

codspeed-hq bot commented Oct 31, 2023

CodSpeed Performance Report

Merging #1051 will improve performances by 14.35%

Comparing Luca-Blight:main (558fdf8) with main (cb47b96)

Summary

⚡ 2 improvements
✅ 138 untouched benchmarks

Benchmarks breakdown

Benchmark main Luca-Blight:main Change
test_validate_literal[python-few_mixed] 32.3 µs 28.4 µs +13.82%
test_validate_literal[json-few_mixed] 35.3 µs 30.9 µs +14.35%

@Luca-Blight Luca-Blight changed the title Adopt ruff formatter instead of black WIP Adopt ruff formatter instead of black Oct 31, 2023
@Luca-Blight Luca-Blight changed the title Adopt ruff formatter instead of black Adopt ruff formatter instead of black (ready to review) Nov 1, 2023
@Luca-Blight Luca-Blight changed the title Adopt ruff formatter instead of black (ready to review) Adopt ruff formatter instead of black (ready for review) Nov 1, 2023
Copy link
Member

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

@Luca-Blight,

Looks good overall - could you run locally to make sure all is working as expected? 👍 Thanks for the help!

Also, the test that's failing isn't related to your work, so don't worry about that.

Makefile Outdated
Comment on lines 93 to 98
$(black)
$(ruff) --fix --exit-zero
ruff format --check --diff $(sources)
cargo fmt

.PHONY: lint-python
lint-python:
$(ruff)
$(black) --check --diff
ruff --fix --exit-zero $(sources)
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll want to do something like we did with pydantic here:

https://github.com/pydantic/pydantic/blob/9868b456e4352558aa01217a6688b30554c6f290/Makefile#L26-L34

With a ruff --fix and ruff format call in the format step and ruff and ruff format check in the lint step 👍

Copy link
Member

Choose a reason for hiding this comment

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

Correct, lint should change anything.

Surely ruff format should imply ruff --fix?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should use both based on this: https://docs.astral.sh/ruff/formatter/#conflicting-lint-rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sydney-runkle

My thinking is there was some redundancy in the last PR.
As an example, a linter and formatter command under the linting step:

.PHONY: lint  ## Lint python source files
lint: .pdm
	pdm run ruff $(sources)
	pdm run ruff format --check $(sources)

Nonetheless happy to make the modifications per your recommendation.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think we're really using lint like a check step and the format step like a fix step. I'm making some changes to pytest-examples right now that should iron out the details of which things we want to use where.

I'll get back to you on Monday with an answer about which we want to use here 👍

Copy link
Member

Choose a reason for hiding this comment

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

Let's stick with the same pattern we're using in pydantic for now 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, let me know if anything else.

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks!

@davidhewitt davidhewitt enabled auto-merge (squash) November 9, 2023 09:22
@davidhewitt davidhewitt merged commit 5712edf into pydantic:main Nov 9, 2023
30 checks passed
@Luca-Blight Luca-Blight changed the title Adopt ruff formatter instead of black (ready for review) Adopt ruff formatter instead of black Nov 9, 2023
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.

Adopt ruff formatter instead of black
4 participants