openzim / python-scraperlib

Collection of Python code to re-use across Python-based scrapers
GNU General Public License v3.0
19 stars 16 forks source link

Migrate to python bootstrap conventions #128

Closed benoit74 closed 7 months ago

benoit74 commented 7 months ago

Rationale

Fix #118 Fix #120

Changes

Important notes

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (4dc3012) 100.00% compared to head (ad4b241) 100.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #128 +/- ## ========================================== Coverage 100.00% 100.00% ========================================== Files 32 32 Lines 1319 1332 +13 Branches 0 225 +225 ========================================== + Hits 1319 1332 +13 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rgaudin commented 7 months ago

at some point, we will be able to move this to python-libzim

FYI this was not done before because of unsupported types syntax in Cython file. I know things has improved on their side lately so this must be re-assessed. But you're right to add it here for now

rgaudin commented 7 months ago

dependencies have been pinned

That's a mistake. scraperlib is a library and this renders using it very inflexible. See https://github.com/openzim/_python-bootstrap/wiki/pyproject.toml#dependencies

benoit74 commented 7 months ago

dependencies have been pinned

That's a mistake. scraperlib is a library and this renders using it very inflexible. See Wiki: pyproject.toml (dependencies) (openzim/_python-bootstrap)

🤦🏻 OMG

That been said, it remembers me that I forgot to specify one thing: the required version in setuptools was open-endeed (i.e. >=3.7). Was this intentional? (hatch-openzim does not support it ...)

benoit74 commented 7 months ago

As discussed live:

RUF012 should be changed to declare the ClassVar. We don't need mutable dict for presets but it's more readable than using unmutable types.

I prefer the MappingProxyType and will use this approach

  • FBT002 I think it's a really useful rule… but applying it is a breaking change. Can we open a ticket and switch for next major?
  • Next for N818 (NotFound exc)
  • We could also then rename getLogger that was chosen to mimic logging's but we dont really care about that.

N802 and N803 might be considered as well, except the ones of _libkiwix and maybe the ones of config_metadata, I will open a ticket

no need for runinstalled as we're always installed using our bootstrap

Removed

as discussed, we should aim for 100% coverage, as it was

I will fix (it is done indeed now)

benoit74 commented 7 months ago

I prefer the MappingProxyType and will use this approach

I finally changed my mind and used ClassVar which is semantically more correct I think

Test coverage is now 100%.

I had to disable the test_user_agent test because it looks like there is a name resolution issue on Github Actions: https://github.com/openzim/python-scraperlib/actions/runs/7869193867/job/21467889432?pr=128 ; I've opened an issue to not forget to reactivate it asap: https://github.com/openzim/python-scraperlib/issues/129

benoit74 commented 7 months ago

I see you've added a bunch of tests. Was that all mandated by the new conditional behavior of coverage?

Not all of them, in same case one test was sufficient to provide 100% coverage but it made no sense to not write more tests while working on it (e.g. for test_make_zim_file_working, I'm pretty sure it wasn't needed to test all four combinations to have 100%). But the only driver to look at writing more tests was a situation which wasn't covered by a test.