search

beetbox / beets

music library manager and MusicBrainz tagger
http://beets.io/
MIT License
12.78k stars 1.82k forks source link

Replace setup.py packaging by Poetry #5266

Closed snejus closed 3 months ago

snejus commented 4 months ago

Migrate beets package configuration to Poetry which nowadays seems to be the gold standard.

I have been using Poetry since 2019 and I have mostly been happy a happy user: it makes local dev setup easy and has the tools I need to maintain python packages day to day, including reliable dependency resolution, versioning and publishing to Pypi.

It's a user-friendly tool, so it should make it more straightforward for contributors to setup and navigate the codebase, and ultimately, hopefully facilitate more frequent releases!

Since poetry manages local virtual environment, we do not have much need for tox any more. Therefore, it was replaced by a task runner poethepoet. Type poe in the project directory to see the available commands.

github-actions[bot] commented 4 months ago

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

Serene-Arc commented 4 months ago

I'm fine with this change to poetry in theory, once all the tests and linting errors have been fixed, but a couple of things:

snejus commented 4 months ago

I'm fine with this change to poetry in theory, once all the tests and linting errors have been fixed, but a couple of things:

  • What are the purposes of the changes to release.py? A lot of the things that I removed seem to have been added back in, some of which are definitely not supposed to be there?

These commits have been made before I noticed that release.py has been adjusted on master - it needs rebasing where it will get removed :)

  • What are the purposes of changing the tox file, specifically calling binaries instead of python -m black for instance?

Some of the changes have to do with tox upgrade to 4.0.0. Regarding calling the binaries directly - that's the accepted standard, given that every command will be run within the tox environment, where it will be resolved automatically. It is slightly shorter as well.

  • There are some issues with changing version in the release workflow; does this affect any of that?

When I made changes here, the release workflow did not exist yet, so I made adjustments to release.py instead. When I rebase, I will make sure to update the release workflow instead.

Serene-Arc commented 4 months ago

These commits have been made before I noticed that release.py has been adjusted on master - it needs rebasing where it will get removed :)

Fair enough!

Some of the changes have to do with tox upgrade to 4.0.0. Regarding calling the binaries directly - that's the accepted standard, given that every command will be run within the tox environment, where it will be resolved automatically. It is slightly shorter as well.

Great, was just worried with path stuff, but if it's only for the tox environment internally, makes sense.

When I made changes here, the release workflow did not exist yet, so I made adjustments to release.py instead. When I rebase, I will make sure to update the release workflow instead.

Cool.

With regards to the release.py changes, do you want to keep them here or move them to #5274? It might make more sense to get the release workflow working for the current project before doing a big tool change like poetry.

snejus commented 4 months ago

do you want to keep them here or move them to https://github.com/beetbox/beets/pull/5274? It might make more sense to get the release workflow working for the current project before doing a big tool change like poetry.

I'm completely with you here @Serene-Arc - I'm moving the relevant commits over.

Serene-Arc commented 3 months ago

@snejus I made some changes to the actions. Everything else looks good so if you could review the changes, I think it's ready to merge!

snejus commented 3 months ago

Let's have a look!

snejus commented 3 months ago

Just noticed that neither of the workflows managed to run due to some issues: image

Serene-Arc commented 3 months ago

Yeah I didn't do full testing on the actions yet, since they're very finnicky and rely a lot on what repo they're running on. Wanted to see if you're happy with the changes before I spend that extra time x.x

Serene-Arc commented 3 months ago

Thanks for fixing up my code!

The one thing I can see is that you've regressed the commit changing the paths of the format commands, format and the format-check. I still think that they should be made explicit as to what directories to change. I think it'll make the project less likely to somehow get non-standard code added by mistake and it will mean that virtual environments don't get arbitrarily formatted. Perhaps adding the docs and extra/release.py to the previous folders would work?

Other than that, I think it's good to be merged.

snejus commented 3 months ago

Thanks for fixing up my code!

The one thing I can see is that you've regressed the commit changing the paths of the format commands, format and the format-check. I still think that they should be made explicit as to what directories to change. I think it'll make the project less likely to somehow get non-standard code added by mistake and it will mean that virtual environments don't get arbitrarily formatted. Perhaps adding the docs and extra/release.py to the previous folders would work?

Other than that, I think it's good to be merged.

Unfortunately neither black nor isort has configuration for default paths to check that could be overridden by command-line arguments, like mypy has. This means that if we hardcode beets, beetsplug etc. these tools will always run across the entire codebase, and thus we won't be able to check the changed files only. Added a comment above the tasks' definition to clarify this.

You're making a good point regarding virtual environments and other unwanted files - luckily both black and isort by default ignore everything defined in .gitignore. Since our .gitignore has .venv and venv, those files should not get formatted 👍🏼 if we find that locally something unexpected is getting reformatted, we can just add those paths to .gitignore too since they should most likely be ignored by git as well.

Serene-Arc commented 3 months ago

I'm not sure that checking against the changed files only is the best way to do this. Like when we did the release, it is technically possible for unformatted code to make its way into the codebase, however that happens. Currently, any code will always be found by the next PR because it'll come up with a change to the file or a format error in the CI. If we make it so only changed files are checked, this code can be missed for who-knows-how-long.

Maybe it's paranoid but I prefer the formatting tools checking the entirety of the codebase every time. It makes sure that the formatting rules are actually being enforced at every stage possible.

snejus commented 3 months ago

Thanks for clarifying @Serene-Arc, I now see what you mean! Given that the code base is already neat and formatted, I agree that checking everything every time makes more sense! I'm adjusting it now.

I think the only-check-what-has-changed approach makes more sense when the code base is not yet formatted, and is getting tidied up iteratively (if you adjust a file, you must format it properly), which is what we did in most cases in my previous experience. 😅

snejus commented 3 months ago

As you predicted @Serene-Arc, there was indeed an unformatted file docs/conf.py that would have been missed! It's fixed now, thanks for your persistence regarding this change! <3

Serene-Arc commented 3 months ago

Wonderful! That's all my concerns fixed then, merge whenever you'd like.

snejus commented 3 months ago

Ah, I'm only seeing your review comments now, but I guess they are outdated now! 😅 I'm just looking at the last bit to do with the docs linting job, and will merge once it's working as expected.