neogeny / TatSu

竜 TatSu generates Python parsers from grammars in a variation of EBNF
https://tatsu.readthedocs.io/
Other
408 stars 48 forks source link

RFC: Support for Python versions from 3.8 to 3.10 #270

Closed dnicolodi closed 2 years ago

dnicolodi commented 2 years ago

I would like to support all currently supported Python version (this means 3.7 to 3.10, but I would be more than happy with versions 3.8 to 3.10) in a project where I would also like to use TatSu. TatSu release 5.7.0 is advertised (at least in the wheel metadata) to support these Python version, however, it does not work with anything older than Python 3.9 as it uses functools.cache.

I decided then to see which features or syntax of which Python release are used in the current master branch. Turns out that there isn't anything that prevents to use Python 3.8 and later as the use of functools.cache reported above is gone. Actually, there are just a few minor adjustments required to support Python 3.7, namely the syntax for mandatory keyword arguments and one instance of fancy f-string interpolation, but the mandatory keyword arguments syntax is indeed nice to have, thus I am not advocating to support such an old Python release.

Could extending the supported Python releases to 3.8 and later be considered until features of syntax introduced in later Python releases is actually required? It would really make the life of users of the library much easier. I am more than happy to help supporting older Python versions if any problem should arise.

If extending support for older Python releases is seen as hampering development of TatSu 5.8, I would also happy with a release from the 5.7 branch with the relaxed Python version requirement. In this case I would be happy to help backporting any bugfix happening in the 5.8 branch to the 5.7 branch.

apalala commented 2 years ago

A branch release from 5.7 is possible, but:

dnicolodi commented 2 years ago

Thank you for having a look at this PR (and to the other PRs).

  • this pull request doesn't seem to cover all the required changes

As far as I know, the only thing missing are adjustments to the project documentation. Do you have something else in mind?

  • you would have to maintain it

Sure, I can take care of this, once we define what this branch should contain. See below.

  • I just created a py38_compatibility branch as target for ongoing work

I see that the branch is branched from master, thus after the 5.8.0 release. Should an hypothetical release supporting older Python versions be 5.8.0 plus required changes to support older Python versions or the latest 5.7 plus changes required to support old Python versions plus fixes implemented in the 5.8 branch? What version number should be assigned to a release from this branch?

apalala commented 2 years ago

There have been several bug fixes and optimizations in the past months. The Py38 compatible version should branch from the latest stable, and then apply the changes required for Py38, which are not only documentation:

If adding py38 to tox.ini works, the release could be v5.7.z or v5.6.z, as it's legal to back-port improvements into older versions (it is done for security fixes for older Python versions).

I'm almost certain that I've started using the new syntax in places, so my expectation is that tox will currently not pass on py38.

dnicolodi commented 2 years ago

apply the changes required for Py38, which are not only documentation:

* Makefile

I don't see anything Python-version specific in the Makefile

* tox.ini

The CI tests do not use Tox and I am not familiar with the tool, thus I didn't update tox.ini but I can guess what it takes to extend the tox environment definitions to cover Python 3.8 and 3.9. Patch coming soon.

* .github/workflows/default.yml

This is done in one of the patches above. It enables tests for all python versions from 3.8 to 3.10. No changes to the code required. I also use TatSu on a project and I run the project tests on the same Python versions (forcing the installation of TatSu with the --ignore-requires-python argument to pip) and all tests pass.

dnicolodi commented 2 years ago

I changed the target branch for this PR from py38_compatibility back to master because the former does not contain some fixes that are already in master and that make the tests fail and I don't know yet how you want to handle fixes backports.

apalala commented 2 years ago

@dnicolodi, please create a pull request to add your contributions in the docs? https://tatsu.readthedocs.io/en/stable/credits.html

apalala commented 2 years ago

We can't have this on master. The steps could be:

dnicolodi commented 2 years ago

Would you like to elaborate on why this cannot be merged on master? Following your procedure above we would end up with a 5.7.x release that is identical to the 5.8.1 release except for one line in setup.cfg. I think it would be a quite strange versioning choice, but if this is what is desired, I don't have objections and I can keep merging commits from master to the py38_compatibility branch in the future and prepare 5.7.x releases.

However, I would like to have the first commit in this PR merged into master.

dnicolodi commented 2 years ago
  • merge master into this pull request and on the py38_compatibility branch
  • edit tatsu/_version.py so there's a 5.6.Z or 5.7.Z version number
  • make sure all tests run

These steps are done.

apalala commented 2 years ago

My apologies, @dnicolodi. You need to merge once more from py38_compatibility to reduce the changelog.

You didn't change 24 files. I did.

dnicolodi commented 2 years ago

You didn't change 24 files. I did.

I see you merged the PR after this comment. Sorry if something was wrong with the PR, I usually don't merge into branches with PRs and I may have done something wrong. It was not my intention to rewrite the git history to make it look like the commits were mine.

apalala commented 2 years ago

There's still work to do to publish. Maybe a new pull request?

I just published TatSu, so I'll merge stable into py38_compatibility.

dnicolodi commented 2 years ago

Please see the other PRs I recently opened. I merged master into py38_compatibility in #283