Skip to content

Conversation

@leiserfg
Copy link
Collaborator

  • Add pre-commit and ruff
  • Automated fixes
  • Apply ruff modernization
  • Fix remaining errors by hand
  • Set python minumum version to 3.10

Fixes # .

Changes proposed in this pull request:

Attention: @prkumar

@leiserfg
Copy link
Collaborator Author

@prkumar can you re-enable the tests to be sure I didn't break anything? I would like to also move from setup.py to pyproject.toml so we can use uv.

@leiserfg
Copy link
Collaborator Author

leiserfg commented Mar 1, 2025

I ran the tests in python3.10 and these are the only ones failing, but as I don't uderstand well this part of the internals I will need some help to fix them:

FAILED tests/integration/test_handlers.py::test_error_handler_with_consumer - tests.integration.test_handlers.WrappedException
FAILED tests/integration/test_handlers.py::test_error_handler - tests.integration.test_handlers.WrappedException
FAILED tests/integration/test_retry_aiohttp.py::test_retry_with_asyncio - RuntimeError: no running event loop

@prkumar
Copy link
Owner

prkumar commented Mar 8, 2025

Hi, @leiserfg - Thanks for this great PR! I tried to reenable the Travis CI builds, but ultimately decided to migrate our CI pipeline to GitHub Actions. Could you try rebasing this PR onto master?

@prkumar
Copy link
Owner

prkumar commented Mar 8, 2025

Hey, @leiserfg - Was trying to be helpful and pushed a change to the PR to fix a merge conflict I caused. Seeing now that you also force-pushed a change to fix the conflict. Sorry if my change caused any hassle! I'll hold off on pushing any additional changes.

@leiserfg
Copy link
Collaborator Author

leiserfg commented Mar 8, 2025

👍 I just rebased as you toldme, I think you better do there what you need, I don't care if that removes my authorship on the commits as long as that leads to a better uplink.

@prkumar
Copy link
Owner

prkumar commented Mar 8, 2025

Do you know what changes we need to fix the tests?

I'm guessing that py.test needs to be updated to pytest in https://github.com/prkumar/uplink/blob/master/tox.ini#L9

Also, any preference for keeping tox or moving to something else? I know you mentioned switching from pipenv to uv, which sounds good to me. Does uv provide an alternative to tox?

@leiserfg
Copy link
Collaborator Author

leiserfg commented Mar 8, 2025

There is https://github.com/tox-dev/tox-uv that allows using the same tox config and leveraging uv for creating the environments. But I think we better keep it as is by now so we can focus on passing the tests and then I can do the changes to uv.

@leiserfg
Copy link
Collaborator Author

leiserfg commented Mar 8, 2025

🤦 I forgot that I already added UV. Then let me check if I can make the test run.

@prkumar
Copy link
Owner

prkumar commented Mar 8, 2025

Sounds good! Let me know if you need any help with getting the tests to run.

@leiserfg
Copy link
Collaborator Author

leiserfg commented Mar 8, 2025

@prkumar done. As I mentioned a couple of tests are failing but I don't know that part of the code so I'm not sure how to fix them.

@prkumar
Copy link
Owner

prkumar commented Mar 8, 2025

@leiserfg - Fixed the tests!

@prkumar prkumar self-requested a review March 8, 2025 22:18
@prkumar
Copy link
Owner

prkumar commented Mar 8, 2025

Changes look good to me! TY! Feel free to merge :)

@leiserfg leiserfg merged commit 1c8727d into prkumar:master Mar 9, 2025
5 of 6 checks passed
@leiserfg leiserfg deleted the modernize branch March 9, 2025 07:10
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