wagtail / wagtail-localize

Translation plugin for Wagtail CMS
https://wagtail-localize.org/
Other
226 stars 86 forks source link

Use Flit for packaging #589

Closed engineervix closed 2 years ago

engineervix commented 2 years ago

Starting with PEP 621, the Python community selected pyproject.toml as a standard way of specifying project metadata. Given that the future of Python packaging is pyproject.toml (based on PEP 518 and PEP 621, transitioning Python projects to use pyproject.toml is inevitable.

This PR introduces Flit as a packaging tool for the project. Flit prides itself in its provision of a simple way to create and upload pure Python packages and modules to PyPI. It focuses on making the easy things easy for packaging.

TODO:

codecov-commenter commented 2 years ago

Codecov Report

Merging #589 (78dde34) into main (ebde7a3) will decrease coverage by 0.08%. The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #589      +/-   ##
==========================================
- Coverage   91.60%   91.51%   -0.09%     
==========================================
  Files          47       47              
  Lines        3907     3913       +6     
  Branches      593      595       +2     
==========================================
+ Hits         3579     3581       +2     
- Misses        186      188       +2     
- Partials      142      144       +2     
Impacted Files Coverage Δ
wagtail_localize/views/edit_translation.py 85.25% <0.00%> (-0.60%) :arrow_down:

Continue to review full report at Codecov.

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

chris48s commented 2 years ago

Nice. I'll have a proper look over this next week, but I'll flag a couple of quick things now. One is we need to get the netlify builds working:

https://github.com/wagtail/wagtail-localize/blob/main/netlify.toml

Tbh, it should probably work as-is if we upgrade pip:

pip install --upgrade pip
pip install -e.[testing,documentation]

but it would be more idiomatic to use filt. Looks like the equivalent would be:

pip install flit
flit install --extras testing documentation

(untested)

Also it looks like the Github Actions CI builds are passing, but again it would probably be nicer to invoke flit than pip here

and we're going to want to create our cache keys based on hashing pyproject.toml, not setup.py

Can you add those to the list of next things to look at.

As I say, I'll have a more detailed look next week.

chris48s commented 2 years ago

Looks like the equivalent would be:

pip install flit
flit install --extras testing documentation

(untested)

Just corecting myself here:

Having played with it locally, the default for flit install is actually --deps all so it will install all optional dependencies by default. This is actually pretty reasonable for local dev and in line with what tools like NPM and poetry do. In CI it might sometimes be useful to just install the subset we need for performance reasons, so actually what we want to do here is explicitly flit install --deps production when we only want the required packages (so we can install fewer things for fast builds when we don't need the optional packages) and then flit install --deps production --extras documentation,testing or flit install --deps production --extras google when we do want specific optional packages.


On Documentation:

Having chewed this over, one nice property of using flit is that if you have a sufficiently recent version of pip and you don't need to build/publish the package, you don't actually need to care that we are using flit as our build system. i.e: even when we migrate to flit

git clone git@github.com:wagtail/wagtail-localize.git
cd wagtail-localize
pip install -e .[testing] -U

will still get you a working development environment that you can use to contribute a feature/bugfix.

Given that, I wonder if we should change the docs at all, or maybe document:

### Using pip

...

### Using flit

...

We can actually make flit mainly a concern for maintainers and not something a casual contributor needs to worry about, which is probably good for community contributions.

Given that it probably makes sense for anything we're expecting to run locally (e.g: https://github.com/wagtail/wagtail-localize/blob/b7ebc96b1b9d852e1110b20bd8707e4aece887b0/tox.ini#L18 ) to continue to run pip rather than flit so you don't actually need it to be installed.

chris48s commented 2 years ago

I don't want this to spill past the end of the rock quarter so I've just pushed the commits to address the review comments myself