-
Notifications
You must be signed in to change notification settings - Fork 271
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
Upgrade tooling on the project. #1329
base: master
Are you sure you want to change the base?
Conversation
- Replace black by ruff, as it's quicker ; - Use `uv` wherever possible as a replacement for pip, as it's way faster to run, add an `uv.lock` file which will be synced before the releases and published here ; - Remove tox, it's too complex for this project and can easily be replaced by `uv` ; - Apply `ruff` formatting ; - Update the makefile accordingly ; - Update the CI accordingly
Tests are failing with python 3.10 + sqlite, but I cannot reproduce locally with the following command: TESTING_SQLALCHEMY_DATABASE_URI="sqlite:///budget.db"; uv run --python 3.10 --extra dev --extra database pytest . Edit: It's fixed after a re-run. |
Curious about your feelings on this @zorun ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still alive, and sometimes, I even have time to review!
Co-authored-by: Glandos <bugs-github@antipoul.fr>
Co-authored-by: Glandos <bugs-github@antipoul.fr>
Keep in mind that I tend to be conservative for this kind of reviews ;) I once inherited a project using poetry and I really didn't like it. Here are general questions:
|
No problem, thanks for the questions. I believe part of the motivation behind this change is personal: putting back this project as a way to experiment with "new shiny tools". It gives me energy, and as such I believe it will be net-beneficial for the project in the end. More fun means it will get easier to maintain (at least for me, but you can have a different opinion). With that out of the way, let's discuss the points you bring here 👍
I believe the main pro is speedup. I personally also like to be able to install different versions of python in a simple way with
I don't believe we have performance issues with the current tooling, no.
My feeling is that
No, not really. It can affect how we release, because the tools we use would be installed by
Yes.
When using pip, the dependencies are not compiled by the
I don't think there is a need for it :-)
It's mainly a DX change.
I believe it's actually the other way around, as it makes things simpler and faster. |
Thanks, I wanted to hear your motivation. If it motivates you, that's good, but let's not break things for free. Actually, thinking about it, I think my motivation comes from repairing broken things, so maybe we make the perfect combo :D
This is a good argument.
Let me be more clear, because this is a general grip I also had with poetry. Lockfiles are very good when working between developers: we are sure we have the exact same version of dependencies. However, users will have different dependencies because they don't use the lockfiles, they use pip. If we run tests with the dependencies from the lockfile, we don't test what the actual users will install, and this is quite bad. We could run tests using the dependencies from pyproject.toml instead of the lockfile, but then, what would be the point of the lockfile? In that case, let's just use pyproject.toml also for development. What do you think? Did I miss something obvious? |
🙌 🤭
Oh, that's nice, and I understand better now the reason why we have the "minimal" set of dependencies in our tests. I actually think this is a good idea. I see the For instance, for the Docker image, we could ensure that it ships with exactly the versions that were present at the time of the build, and I believe that would be a good thing. Not a requirement, but something useful for some people. But, to be honest, these |
Also: thanks for the tone of the discussion, I told you already but it's been a pleasure to disagree with you, and find out how we can better understand each other and come to the same conclusion. (This is probably because we're not trying to push our decisions onto each other, but really care about what we think, and why) |
It's also my point-of-view. Since I knew nothing about
I remember taking part in this, and this was a really good move, because it also tests installation in every cases. I also found myself in situation where I needed to install something (hello Ansible) at not the very last version, and was happy it worked :) |
Hey, @zorun, after some rest, what's your feeling on this? Should we merge or close? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed on principle, but there are some unknowns for me. I have left a few comments.
Here is the main missing stuff, I think:
- did you check that all instructions in
docs/contributing.md
still work after this change? - what's the position on the uvlock? As I said, I'm a bit dubious about it because users will install IHM via pip, so I believe we shouldn't use the uvlock at all. Does uv use the lock by default? Once we reach a consensus, please document the result somewhere in the doc.
- should the Dockerfile use uv or continue using pip?
|
||
test: | ||
# Dependency on linting to avoid running our expensive matrix test for nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you remove this comment and the other ones below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure! (tbh I don't remember). Let's add them back.
- name: Change dependencies to minimal supported versions | ||
run: sed -i -e 's/>=/==/g; s/~=.*==\(.*\)/==\1/g; s/~=/==/g;' pyproject.toml | ||
run: sed -i -e '/requires-python/!s/>=/==/g; /requires-python/!s/~=.*==\(.*\)/==\1/g; /requires-python/!s/~=/==/g;' pyproject.toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? Maybe add a comment.
if: matrix.database == 'mariadb' | ||
env: | ||
TESTING_SQLALCHEMY_DATABASE_URI: 'mysql+pymysql://root:ihatemoney@localhost:3306/ihatemoney_ci' | ||
TESTING_SQLALCHEMY_DATABASE_URI: ${{ matrix.database == 'sqlite' && 'sqlite:///budget.db' || matrix.database == 'postgresql' && 'postgresql+psycopg2://postgres:ihatemoney@localhost:5432/ihatemoney_ci' || 'mysql+pymysql://root:ihatemoney@localhost:3306/ihatemoney_ci' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't find this very readable, what's wrong with the current approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a comment there. It's just a different approach, that I find better because it has less repetition. I can surely add a comment here and format it better so it's easier to parse :-)
No, not yet. I will if we decide to go this way.
I believe But, it won't be used for installations via pip. I think we should keep it, but it won't have any effect on the distribution via PyPI. |
uv
wherever possible as a replacement for pip, as it's way faster to run, add anuv.lock
file which will be synced before the releases and published here ;uv
;ruff
formatting ;I'm not sure why, but
pytest-libfaketime
is currently not working when running insideuv
, at least on an apple silicon machine. I uninstalled it otherwise the test couldn't run, failing with the following error: