Skip to content

Added uv migration and format codebase with ruff#780

Closed
josecsotomorales wants to merge 3 commits intomasterfrom
migrate-to-uv-and-ruff
Closed

Added uv migration and format codebase with ruff#780
josecsotomorales wants to merge 3 commits intomasterfrom
migrate-to-uv-and-ruff

Conversation

@josecsotomorales
Copy link
Collaborator

No description provided.

@josecsotomorales josecsotomorales changed the title Migrate to uv and ruff Added uv migration and format codebase with ruff Jul 9, 2025
@kurtmckee
Copy link
Collaborator

This is an enormous change and cannot be reviewed for what it is. You've also failed to document the things that you've modified, like re-adding Python 3.8 support, which is EOL.

@kurtmckee kurtmckee closed this Jul 9, 2025
@kurtmckee kurtmckee deleted the migrate-to-uv-and-ruff branch July 9, 2025 21:52
@josecsotomorales josecsotomorales restored the migrate-to-uv-and-ruff branch July 10, 2025 00:59
@josecsotomorales
Copy link
Collaborator Author

josecsotomorales commented Jul 10, 2025

@kurtmckee, not sure why it can't be reviewed, all changes are related to ruff formatting. The goal of this PR is to migrate this project to uv and ruff, and all tests are passing. I agree and will remove all Python 3.8 support changes. For the documentation feedback, I'm more than happy to add it.

@josecsotomorales
Copy link
Collaborator Author

Rolled back changes related to Python 3.8 support and added changes to: bae5e8f#diff-ff3c479edefad986d2fe6fe7ead575a46b086e3bbcf0ccc86d85efc4a4c63c79

@josecsotomorales
Copy link
Collaborator Author

Here's what actually changed on this PR:

@josecsotomorales
Copy link
Collaborator Author

@kurtmckee I'm happy to review the ruff customizations to try to match what we currently have in master and generate fewer diffs. Still, first, I want to confirm if you guys are open to collaboration, this project is close to abandoned, and I don't want to waste either your time or my time.

@josecsotomorales josecsotomorales force-pushed the migrate-to-uv-and-ruff branch from bae5e8f to 247b299 Compare July 10, 2025 13:21
@kurtmckee
Copy link
Collaborator

@josecsotomorales I'm open to reviewing these changes, and appreciate your effort to modernize the project, which needs a helping hand. However, there are two completely unrelated changes happening here, and it makes the blast radius of this change unnecessarily large.

Opening a PR with thousands of lines of changes and zero documentation via the changelog, the commit entries, or the PR body is unacceptable -- always -- and is why I summarily closed the PR in the first place.

My specific feedback here is that migrating to uv is unrelated to using ruff. I would like the migration to uv to be a separate, initial PR. The ruff changes can be a fast-follow after the migration to uv is reviewed and merged.

I want to emphasize again that I'm grateful for your effort to modernize the project infrastructure, and I appreciate that you've invested your time to help the project! My single request here is that you design the changes to honor my time as a reviewer as well.

@josecsotomorales
Copy link
Collaborator Author

Hey, I’d appreciate it if you didn’t close the PR again — I’m also a maintainer on this project and plan to merge it.

You closed it without reviewing, even though most of the 1K+ lines are from the uv.lock file. There are meaningful improvements in the PR that shouldn’t be discarded so lightly. Let’s work collaboratively — if you have concerns, I’m happy to address them, but auto-closing without discussion isn’t the right approach.

@kurtmckee
Copy link
Collaborator

if you have concerns, I’m happy to address them

I'm very serious that this PR is unacceptable as-is. Please address them by opening a new PR that introduces uv by itself -- no ruff changes. The changes are unrelated.

Please do not open this PR again.

@kurtmckee kurtmckee closed this Jul 10, 2025
@kurtmckee kurtmckee deleted the migrate-to-uv-and-ruff branch July 10, 2025 14:19
@josecsotomorales josecsotomorales restored the migrate-to-uv-and-ruff branch July 10, 2025 14:19
@kurtmckee kurtmckee closed this Jul 10, 2025
@kurtmckee kurtmckee deleted the migrate-to-uv-and-ruff branch July 10, 2025 14:19
Repository owner locked as too heated and limited conversation to collaborators Jul 10, 2025
@josecsotomorales josecsotomorales restored the migrate-to-uv-and-ruff branch July 10, 2025 14:20
@kurtmckee kurtmckee deleted the migrate-to-uv-and-ruff branch July 10, 2025 14:20
@kurtmckee kurtmckee closed this Jul 10, 2025
@josecsotomorales josecsotomorales removed the request for review from kurtmckee July 10, 2025 14:21
@josecsotomorales josecsotomorales restored the migrate-to-uv-and-ruff branch July 10, 2025 14:21
@kurtmckee kurtmckee closed this Jul 10, 2025
@kurtmckee kurtmckee deleted the migrate-to-uv-and-ruff branch July 10, 2025 14:22
@kurtmckee
Copy link
Collaborator

We've entered in a very negative interaction, and I'm sorry this has happened. However, please do not force this change through as-is.

@josecsotomorales
Copy link
Collaborator Author

I get your concern about mixing the uv migration and ruff formatting, but let’s be real: the vast majority of the diff comes from introducing uv.lock, not formatting changes. The actual code changes from ruff touch fewer than 200 lines.

This PR isn’t trying to sneak in unrelated work — it’s a set of clear improvements, and I’m fully open to discussing anything you’d like to split, rework, or defer. But repeatedly closing the PR without engaging on the specifics doesn’t help either of us.

As a maintainer, I’m merging this because I believe it improves the project. Happy to collaborate — just not on a one-way street.

@kurtmckee
Copy link
Collaborator

I understand that the vast majority of changes are happening due to uv.lock getting introduced. The reason I haven't engaged on the specifics is because the shape of the PR is fundamentally incorrect. However, I'll give you some specific feedback.

uv is not actually being introduced -- you've migrated to PEP 621 metadata in pyproject.toml (highly desirable!) but the build-system is still just setuptools, not uv.

Therefore, uv.lock should not be committed to the repo for two reasons:

  1. This PR fails to make uv the build system, so a uv.lock file doesn't make sense
  2. This is a library, and the repo should not include a lock file

A separate PR that captures the PEP 621 metadata changes is desirable, and I hope that you'll take time to split that atomic change out and submit a PR for that work.

@josecsotomorales
Copy link
Collaborator Author

Excellent! Thanks for the feedback, I'm going to create separate PRs for both PEP 621 migration and Ruff 🚀

@josecsotomorales
Copy link
Collaborator Author

PEP 621 metadata changes PR: #781

@josecsotomorales
Copy link
Collaborator Author

Ruff migration PR: #782

Note:

Built on top of #781, which should be merged first

@kurtmckee
Copy link
Collaborator

Nice! I'm able to review 781 first, but will need to return to 782 after hours. Thanks for splitting these up! ❤️

Repository owner unlocked this conversation Jul 10, 2025
@josecsotomorales
Copy link
Collaborator Author

Thank you for your time and support! And apologies for any confusion 🚀

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