brightway-lca / brightway2-io

Importing and exporting for the Brightway LCA framework
BSD 3-Clause "New" or "Revised" License
26 stars 40 forks source link

Fix compatibility with python 3.8 and 3.9 #245

Closed raphaeljolivet closed 7 months ago

raphaeljolivet commented 7 months ago

This is a fix for #244

It's basically changing : type | type2 to Union[type, type2]

Please consider using Tox to ensure broad compatibility with several version of Python.

cmutel commented 7 months ago

@raphaeljolivet Thanks!

Can you give us an indication of why 3.8 compatibility is needed? It is EOL in 2024, and even if we add compatibility, it would limit us to dependencies which are also 3.8 compatible.

I don't remember the details but some of the CI workflows also broke for 3.8 (though maybe this was for another component library). Normally we don't need tox (and everything that comes with it) if the CI tests multiple versions and OSes, right?

raphaeljolivet commented 7 months ago

The compatilibity to Python 3.8 could be optional indeed. I guess the support for 3.9 should be mandatory. I have not looked in detail your CI scripts, but it seem they did not catch this error on 3.9 at least.

I am in the process of releasing a new version of lca_algebraic.

I have updated to Brightway 2.45 (maybe later to brightway25) : And I have setup sone tests on tox. I ran from 3.8 to 3.11 : It failed for 3.8 and 3.9.

It installed without error on python3.8 and python3.9 but failed at runtime.

To me a library should not install if it fails at runtime : It needs to declare the minimal version of Python it requires (mines doesn't do it either yet, I am still learning those things and trying to get it clean).

For 3.8, while looking at it, I found that it only required a couple of change to install and run smoothly, and I thought it was a pitty to not support it : But indeed I don't have the full picture. If maintaining 3.8 is a burden for the dependencies, you could drop its support and advertise the min python version in the manifest of the library.

raphaeljolivet commented 7 months ago

And thanks for your work btw :)

cmutel commented 7 months ago

@raphaeljolivet right back at you :)

cmutel commented 7 months ago

While you're at it, can you update setup.py to require Python 3.8 and higher?