search

django-ftl / fluent-compiler

High performance Python implementation of Fluent, Mozilla's l10n language
Other
21 stars 4 forks source link

Remove the `ast_compat` module #19

Closed samylovma closed 1 year ago

samylovma commented 1 year ago

For Python 3.6-3.12, there are no compatibility issues with ast (see #18). The nearest planned breaking changes of ast will be in Python 3.14. So I suggest to remove ast_compat as unnecessary code that also needs to be maintained.

samylovma commented 1 year ago

Trying to fix all deprecation warnings of ast (that will be removed in Python 3.14) breaks tests only for Python 3.6-3.7 (see #20, ci). I think this library will not support Python 3.6-3.7 when 3.14 is released.

spookylukey commented 1 year ago

I'm not sure what the motivation is for this change. It has been useful to have this layer in the past, and it is quite likely that we may need to reintroduce it or something similar in the future. It terms of code that needs to be maintained, it is tiny, and extremely low complexity, since it is just a list of aliases, with almost nothing that needs to "be maintained" - until the point where we need it again. I don't think it will give type checkers any problems either, they should all be able to cope with this kind of alias very easily. So there don't seem to be any gains from removing it.

In terms of supporting Python versions, my policy has generally to be more generous than the Python EOL schedule, as long as it costs us relatively little. This is because I've benefitted in the past from libraries with a large compatibility range - it helps projects that have got behind, giving them more flexibility in doing upgrades.

So I tend to drop a Python version only when both 1) it is out of security EOL, and 2) we've got a specific significant reason to want to drop it.

There are other good reasons to keep on to Python 3.6 for now - for example, fluent.runtime currently supports Python 3.6, so for the sake of someone who wants to switch, we might make life easier for them if we match that.

samylovma commented 1 year ago

I agree with you! It's really nice to have a large compatibility range. My motivation was that for ast_compat is unnecessary for Python 3.6-3.12, but I didn't think about the future in case of a large compatibility range.

Thanks!