has2k1 / plotnine

A Grammar of Graphics for Python
https://plotnine.org
MIT License
3.92k stars 210 forks source link

Use string literal types in to_rgba() for matplotlib <3.8 #751

Closed khaeru closed 5 months ago

khaeru commented 5 months ago

Hi—thanks for the fresh 0.13.0 release and continued improvement of the package 🙏🏾

Today we saw errors like this in GitHub Actions workflows that rely on plotnine, at this statement: https://github.com/has2k1/plotnine/blob/f5c01deb8e76e2fb9d0404a7fac527925276ac38/plotnine/_utils/__init__.py#L576

These boil down to the fact that (a) matplotlib.typing does not exist in matplotlib <3.8 but (b) plotnine is compatible with matplotlib >=3.6: https://github.com/has2k1/plotnine/blob/f5c01deb8e76e2fb9d0404a7fac527925276ac38/pyproject.toml#L26

In short:

$ pip list | grep -E "plotnine|matplotlib"
matplotlib                             3.7.0
matplotlib-inline                      0.1.6
plotnine                               0.13.0.post1+gf5c01deb            /home/khaeru/vc/u/has2k1/plotnine

$ python -c "from plotnine._utils import to_rgba; to_rgba(None, None)"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/khaeru/vc/u/has2k1/plotnine/plotnine/_utils/__init__.py", line 576, in to_rgba
    from matplotlib.typing import ColorType
ModuleNotFoundError: No module named 'matplotlib.typing'

This one commit fixes by removing the import and using string literal types within the function. The type checker should then use ColorType as imported within the if TYPE_CHECKING: block at the top of the file.

codecov[bot] commented 5 months ago

Codecov Report

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

Comparison is base (f5c01de) 86.97% compared to head (47d2dc2) 86.97%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #751 +/- ## ========================================== - Coverage 86.97% 86.97% -0.01% ========================================== Files 179 179 Lines 11559 11558 -1 Branches 2116 2116 ========================================== - Hits 10053 10052 -1 Misses 1017 1017 Partials 489 489 ```

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

khaeru commented 5 months ago

Also, FWIW, in some GitHub Actions workflows I maintain I test against both the earliest and latest versions of key dependencies, for instance here. (Some projects use tox for the same purpose.)

I am not familiar with your CI strategy here so I did not hazard to make similar changes, but it can help catch such issues early. Please let me know if you'd like me to try for this PR.

has2k1 commented 5 months ago

Nice. Thanks for this

This one commit fixes by removing the import and using string literal types within the function. The type checker should then use ColorType as imported within the if TYPE_CHECKING: block at the top of the file.

I never thought of using string literals in cast to avoid runtime imports!

has2k1 commented 5 months ago

Also, FWIW, in some GitHub Actions workflows I maintain I test against both the earliest and latest versions of key dependencies, for instance here. (Some projects use tox for the same purpose.)

I considered testing against the against the lowest supported package versions but I felt that I wouldn't keep up with updating the dependencies in two places. The place for these tests is in the weekly tests, which aims to keep track of dependencies.

However Astral's new package installer uv allows installing minimum versions for the dependencies and we can now use that.