Improvement in code quality before the 0.9.x branch was forked

return42 commented 7 months ago

The aim of the project is to improve the code quality before the 0.9.x branch point, before downward compatibility to python 2 is dropped in master. If it only started after the branch point, the diffs between the master branch and the 0.9.x branch would be too large to merge bug fixes downwards.

Improving code quality is an ongoing process. This PR is intended to set the basis for this: adding the checks to the github workflow for PRs ensures that future PRs cannot be merged before they have passed the quality gate.

The tests can be carried out locally as follows (see README.md)::

$ make format pylint test

The github workflows do not support Python 2 versions, the py2 quality gate can therefore only be tested locally and requires a Python 2 runtime (the Makefile assumes asdf is installed / as described in the README.md)::

$ make clean venv2.7 test pylint2.7

[!CAUTION] There are no automated tests for Python 2 on github, the downward compatible 0.9.x releases have to be tested locally !!!

About tools of the quality gate

Tools such as the code formatting of black are version-dependent / new rules are implemented with each new version of the tool. To ensure that the same rules always apply to the quality gate, the versions of the tools must be pinned::

black ==24.3.0; python_version >= "3.8"  # (newest) Black up from py3.8
pylint ==3.1.0; python_version >= "3.8"  # (newest) Pylint up from py3.8
pylint ==1.9.5; python_version =="2.7"   # old Pylint in py2.7

Since we support Python versions that have reached their EOL (py2.7 & Py3.7) we cannot add the tools of the quality gate to every python version we want to support / see remarks about the tools below.

[!CAUTION] Upgrading the version of one of these tools must always be done in a separate PR / could be automated by tools like Renovate or Dependabot.

About Black

Black is not supported in py2 and py3.7 support stopped in Black v23.7.0. Since the Black test is already executed in other Python versions, a further test in py3.7 can be omitted.

We use black started from current version 24.3.0

About Pylint

py2.7: latest pylint version for py2 is pylint v1.9.5

py3.7: since the Python test is already executed in other Python versions, a further test in py3.7 can be omitted.

About linting & code quality

In order to improve the code quality of an existing code base, it is necessary to know it well and identify weak points: Pylint is the right tool for this, it tells you a lot about the code base.

Initially, a profile (a set of rules) should be found that future PRs must adhere to. These rules are defined in the .pylintrc, they essentially correspond to the general standards, with a few exceptions. Typically, the initial application of such a profile to an existing code base reveals many issues.

The quality of these problems varies greatly and is partly determined by the project objectives (e.g. the limits of downward compatibility):

initially, only marginal changes should be made, which are manageable and guaranteed not to result in any functional changes.
Issues reported from the lint that are more far-reaching are initially deactivated via inline comments. To get a full list try: grep '# pylint: disable=' ./*.py ./*/*.py

The latter can be successively eliminated in the following PRs. In this PR, a few thematic groupings are already made, whereby a separate PR has been set up for each topic, which sets the inline comments.

[!IMPORTANT] Read the commit messages along this patch series carefully to understand the story behind.

da4089 commented 7 months ago

I am considering an approach where:

the 0.9.x branch has it's own, different, GitHub actions configuration
we use a standard ubuntu-latest runner
in the workflow, we download and install a Python 2.7 interpreter from a pre-built tarball
the remainder of the workflow would do something like
- python setup.py build
- python setup.py test
- python setup.py sdist
- python setup.py bdist_wheel --universal
we could run pylint 1.9.5 here as well
we could consider running a pinned version of Black that retains Python2.7 support
- I'm not sure there's much value to this. Maybe we just do the formatting once, and thereafter maintain it manually? Anything merged through to the 1.x branch will be blackened by the pre-commit hooks anyway.

return42 commented 7 months ago

I am considering an approach where:

the 0.9.x branch has it's own, different, GitHub actions configuration

Yes, was also my intention .. when branch-point is set we have to modify gh workflow in master and in 0,9.x branch.

we use a standard ubuntu-latest runner

Is, what we already have ..

https://github.com/py-vobject/vobject/blob/8f72c867fa6ce8af14053542298012e67cc4bc94/.github/workflows/test.yml#L4-L5

in the workflow, we download and install a Python 2.7 interpreter from a pre-built tarball

Is something I had also in mind .. but I would give it a try when 0.9.x branch has been created. / I don't want to mix to many things in this one PR.

the remainder of the workflow would do something like

python setup.py build

python setup.py test ...

Lets discuss in a follow up PR.

we could run pylint 1.9.5 here as well

In the 0.9.x branch / was at least my intention.

we could consider running a pinned version of Black that retains Python2.7 support

Nope: see comment from above ..

Tools such as the code formatting of black are version-dependent / new rules are implemented with each new version of the tool. To ensure that the same rules always apply to the quality gate, the versions of the tools must be pinned::

A code formatted by Black v24.3.0 is not accepted by older Black versions / older versions will report errors. This is in the nature of things: the formatting rules are always improved with new versions (they are not static across all versions).

I'm not sure there's much value to this. Maybe we just do the formatting once, and thereafter maintain it manually? Anything merged through to the 1.x branch will be blackened by the pre-commit hooks anyway.

If we have forked the 0.9.x branch, then we can develop a GH workflow that installs python 3 in addition to python 2 and with python 3 you can then also use the Black test ... basically the same way I currently did it (locally) when I developed this PR.

But I think these are all steps that we can take after this PR/ when we have python2 & Python 3 issues separetd in branches.

The only question for me now is whether this PR is a suitable preparation for all of these follow up task.

da4089 commented 7 months ago

I am considering an approach where:

the 0.9.x branch has it's own, different, GitHub actions configuration

Yes, was also my intention .. when branch-point is set we have to modify gh workflow in master and in 0,9.x branch.

<...>

in the workflow, we download and install a Python 2.7 interpreter from a pre-built tarball

Is something I had also in mind .. but I would give it a try when 0.9.x branch has been created. / I don't want to mix to many things in this one PR.

Yes, only after branching is complete.

we could consider running a pinned version of Black that retains Python2.7 support

Nope: see comment from above ..

Tools such as the code formatting of black are version-dependent / new rules are implemented with each new version of the tool. To ensure that the same rules always apply to the quality gate, the versions of the tools must be pinned::

A code formatted by Black v24.3.0 is not accepted by older Black versions / older versions will report errors. This is in the nature of things: the formatting rules are always improved with new versions (they are not static across all versions).

The goal here is to allow easy merging of bug fixes from branch-0.9.x to master. To facilitate that, having both branches with identical, or at least, very similar styles would be helpful.

On the one hand, even an older version of Black will likely have a lot of overlap with more recent versions, so there might be some value in running it. On the other hand, I don't expect to be making major changes in the 0.9.x branch, so ... manually formatting the code in a Black-latest compatible way is probably easily done.

If we have forked the 0.9.x branch, then we can develop a GH workflow that installs python 3 in addition to python 2 and with python 3 you can then also use the Black test ... basically the same way I currently did it (locally) when I developed this PR.

I'm concerned that modern Black uses (or will use in future) formatting that is incompatible with Python 2.7. It might do more harm than good on the 0.9 branch at some point?

The only question for me now is whether this PR is a suitable preparation for all of these follow up task.

I'm mostly inclined to just merge this as is, and work out any issues in master (and/or the 0.9.x branch).

I have two minor concerns about the tooling: introducing a dependency on asdf (I haven't even heard of this before today), and requiring make which might be difficult for contributors running Windows (and not familiar with/willing to use WSL).

Also keen for feedback from others .. @rsb-23?

return42 commented 7 months ago

On the other hand, I don't expect to be making major changes in the 0.9.x branch, so ... manually formatting the code in a Black-latest compatible way is probably easily done.

Yeah, we can decide later / if we see a need for tooling in the 0.9x branch we implement something :)

I'm concerned that modern Black uses (or will use in future) formatting that is incompatible with Python 2.7.

We do not need to upgrade Black in the master branch at the next time / BTW I don't expect any formatting issues related to the py2 vs py3 version .. I have not seen one while I was working on this PR.

I have two minor concerns about the tooling: introducing a dependency on asdf (I haven't even heard of this before today), and requiring make which might be difficult for contributors running Windows (and not familiar with/willing to use WSL).

I totally understand... we can replace the Makefile and asdf after this PR is merged. For now, it was a cheap and quick way to get a quick handle on the various challenges // I wanted to get started and avoid long discussions regarding the appropriate tools at this stage.

I'm mostly inclined to just merge this as is, and work out any issues in master (and/or the 0.9.x branch).

:+1:

da4089 commented 7 months ago

There's also @rsb-23's PR with its use of pre-commit to consider. It has some nice stuff, perhaps it can do some of what make is doing here?

I did a bunch of work on top of that PR also, but perhaps we can "rebase" it once this is merged, and pull in the useful changes?

return42 commented 7 months ago

There's also @rsb-23's PR with its use of pre-commit to consider. It has some nice stuff, perhaps it can do some of what make is doing here?

May be .. its something I had in mind, but I don't have practices and didn't know how I can reconcile the different interests of local testing with those of testing in GH workflows. We will have more build tasks later, not only those that fit a pre-commt (e.g. local build of documentation or packages) ... for this we will need a suitable tool, I don't know yet which one that will be, which is why I would like to move these discussions to dedicated threads.

I did a bunch of work on top of that PR also, but perhaps we can "rebase" it once this is merged, and pull in the useful changes?

The changes from your branch have a large overlap with those from this PR here ... I think it's easiest to just cherry picking after this PR ha been merged (I can assist if you want).

Update: Instead of a tool that is made for pre-commit tasks we should look out for a project focused tool like one of these .. but we should discuss it in a dedicated thread :)

rsb-23 commented 7 months ago

@da4089 - I need a little time (1-2 days) to go through the discussion and changes. Few points based on rough overview:

Number of tools should be as minimum as possible for easy maintenance and contribution.
Lets run pre-commit locally for v0.9.x just for cleanup, but add the config only in v1.0. (due to compatibility concerns)
We can raise PR and merge changes to new branch first and solve any issues before merging to master

Can we move this conversation to GitHub/discussion? It has option to reply instead of quote reply

Since, we have 3-4 things to discuss and it will be easier to follow in threaded conversation.

return42 commented 7 months ago

I have two minor concerns about the tooling: introducing a dependency on asdf (I haven't even heard of this before today), and requiring make which might be difficult for contributors running Windows (and not familiar with/willing to use WSL).

Based on this branch here I implemented a solution that uses environment managemen from Hatch, see PR on my fork:

https://github.com/return42/vobject/pull/2

Update: Instead of a tool that is made for pre-commit tasks we should look out for a project > focused tool like one of these .. but we should discuss it in a dedicated thread :)

hatch

PDM

Rye

See discussion:

https://github.com/py-vobject/vobject/discussions/41

rsb-23 commented 7 months ago

I have two minor concerns about the tooling: introducing a dependency on asdf (I haven't even heard of this before today), and requiring make which might be difficult for contributors running Windows (and not familiar with/willing to use WSL).

If we have just 2-3 scripts to run all the tools, it won't be a problem in introducing dependencies.
Docker seems a good way to handle OS compatibility concerns and run tests for all python versions locally. POC {check extras & script folder too}
All configs should be in pyproject.toml (single file). [We can remove .pylintrc and wherever possible]
We should keep "lint checks" and "unit tests" separate in CI. lint action. Also, we can separate for main branches.
Since linting doesn't impact code working, we should run linters only for python 3.8 and suppress the conflicting changes. It won't be an issue until unit tests pass for all py versions.

.....continued to #41

da4089 commented 7 months ago

@return42 and @rsb-23 -- I'd like to move this forward. I'm not super-familiar with the mechanics of GitHub PRs, but .. can we work in a single branch, and get some of these cleanups applied to master, so we can merge this?

Then we can break of some of these other patches for automation, etc, and apply them after the split.

return42 commented 7 months ago

I think for test_files/*.ics, we should leave them as DOS-style CRLF line endings, because that's what the iCalendar/vCard specs require?

Ooops .. YES should be CRLF / sorry my fail.

I'm not super-familiar with the mechanics of GitHub PRs, but .. can we work in a single branch, and get some of these cleanups applied to master, so we can merge this?

PR's are nothing more than a branch .. with a comment function in GH ..

Then we can break of some of these other patches for automation, etc, and apply them after the split.

We/you can create a new branch and cherry pick commits from the branch of this PR. From this new branch we create a new PR.

While cherry picking you have to slightly modify some of the patches (to solve conflicts and to remove hunks like the Makefile & GH CI).

This patch has already been merged

I assume you refer to bd01503fdce .. / don't cherry pick.

I'm not sure in detail what you want to have in the new branch (PR) .. if you need help to create the new branch & cherry picking and remove hunks ask me.

py-vobject / vobject