Added linters and pre-commit hooks

[rsb-23]

⚠️ Python compatibility 3.7 and above only.

Checks :

• [x] 42 testcases ran successfully. (skipped=1)
• [x] No function names are modified to snake_case.

Changes :

• Added pre-commit hooks along with isort, black, flake8 linters.
• Hooks run automatically during git commit.
• Fixed linting errors and added TODO and noqa for other cases.
• Replaced xrange with range function in win32tz.py

---

🙏🏼 Changes are mostly double quotes, whitespace, indentation and sort order. Apologies, as its too many changes in single PR.

[return42]

Thanks for PR .. why flake8? .. Thought we will use pylint https://github.com/py-vobject/vobject/discussions/27#discussioncomment-8746456 .. to run tests locally we need a solution like running-tests ?

[da4089]

This is great, thank you!

Also, have a look at https://github.com/da4089/vobject/tree/dev-flake8, where I've been working on similar area.

Also, we need to ensure that the resulting code is compatible with Python 2.7 for the 0.9.x releases still. I think I've managed to retain that, but I notice your comment that this is Python3.7+ only?

[da4089]

Thanks for PR .. why flake8? .. Thought we will use pylint #27 (reply in thread) .. to run tests locally we need a solution like running-tests ?

I've been experimenting in my branch with using both, TBH. pylint seems to warn about a lot more stuff than flake8, but maybe both is ok?

I still haven't got to a point of producing useful reporting artifacts from any of these either.

[da4089]

Thanks for PR .. why flake8? .. Thought we will use pylint #27 (reply in thread) .. to run tests locally we need a solution like running-tests ?

I think the best solution for local execution is to put the configuration into pyproject.toml so that it's always consistent, and then perhaps just documentation (like the tests) which says something like:

To run code quality analysis tools, you can simply execute them directly like:
$ flake8 vobject
$ pylint vobject

in the project's home directory.

and then try to make sure the config options in pyproject.toml are universally useful. See the branch above for my first draft attempt at this.

[return42]

See the branch above for my first draft attempt at this.

LGTM at first sight .. give me some more time ..

ensure that the resulting code is compatible with Python 2.7 for the 0.9.x releases still. .. and then try to make sure the config options in pyproject.toml a

Is it compatible with 2.7?

perhaps just documentation (like the tests) which says something like ..

Doc should include instructions to build up a virtualenv / otherwise the local test runs and package versions are highly dependent on the individual Python setup of the developer.

I still haven't got to a point of producing useful reporting artifacts from any of these either.

Not sure I fully understand what you mean by / what you expect .. can you explain it to me in more detail?

---

I hope I have time next weekend to implement a PR that demonstrates what I have in mind ..

[da4089]

I've pushed a change to my https://github.com/da4089/vobject/tree/dev-flake8 branch that:

• Requires Python 3.8 to actually run, but
• Supports Black reformatting the code for a target list including Python 2.7

I think we should be able to safely use this on the 0.9.x code without it breaking stuff like u"foo" strings.

[rsb-23]

Thanks for PR .. why flake8? .. Thought we will use pylint #27 (reply in thread) .. to run tests locally we need a solution like running-tests ?

• There was no final conclusion to use either, hence I started with flake8 as it has lesser breaking rules. Anyway, we can add both.
• For running locally, I use pre-commit run -all for all files and (mostly) git add . && pre-commit run for just modifed files. These command runs all linters together instead of running individually.

[rsb-23]

Thanks for PR .. why flake8? .. Thought we will use pylint #27 (reply in thread) .. to run tests locally we need a solution like running-tests ?

I think the best solution for local execution is to put the configuration into pyproject.toml so that it's always consistent, and then perhaps just documentation (like the tests) which says something like:

To run code quality analysis tools, you can simply execute them directly like:
$ flake8 vobject
$ pylint vobject

in the project's home directory.

and then try to make sure the config options in pyproject.toml are universally useful. See the branch above for my first draft attempt at this.

• I was avoiding extra module flake8-pyproject and used setup.cfg instead. Will move the configs to .toml file.
• pre-commit run -all can be used to run all linters at once, locally.

[da4089]

Thinking ... maybe I should create a branch off master and we can merge this PR into that, and my branch over that, and then sort of any remaining issues before merging the whole lot into master.

Does that sound ok to you?

[return42]

@da4089 I lost the overview about the python releases you want support ..

In the commit 992b8b70@dev-lint you targeting Python2.7 ..

.. but in github CI we test starting with py3.7:

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

Do you plan to add new branch later (after we merged linted code)? A branch for the 0.9.x releases?

[return42]

• For running locally, I use pre-commit run -all for all files and (mostly) git add . && pre-commit run for just modifed files. These command runs all linters together instead of running individually.

@rsb-23 I'm new to pre-commit but it looks interesting .. one thing I miss: we can't combine the linting test/tools (from pre-commit) with other tests needed to be run locally before create a commit / e.g the unit tests (see python tests.py) .. or is there a chance to add unit and other tests from the project to the .pre-commit-config.yaml?

[da4089]

I would like 0.9.x to continue to support 2.7.

I wanted to do some cleanup before splitting, so it's easier to to share patches across the two branches.

master will become 1.0 and support Python 3 only.

I'd like to make the split pretty soon.

[da4089]

I'm new to pre-commit too. I don't have a full understanding of what's possible yet.

[return42]

I would like 0.9.x to continue to support 2.7. I wanted to do some cleanup before splitting,

OK now I got you :) .. but then I think we should first go a step back ...

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

and add back the py2.7 to the github CI. Second we need a way to test code in py2.7 locally (I have https://asdf-vm.com/ in mind).

Reasons why we need to test Py 2.7 (before branching):

1. we do not know what lint tools are work in py2.7 (do we need the tool itself in the branch?)
2. IDK if pyproject.toml is supported well by python's pip & setup tools in Py 2.7 (I have my doubts)
3. I expect that pre-commit will not work in python 2.7
4. All the other things we have not yet in mind, needed to be tested in py2.7

The good news: we only need the code changes before we split a 0.9.x branch ... there is no need to run lint tools in the 0.9.x: if we only transfer patches to the 0.9.x branch from the master, then we don't necessarily need a lint tool in the 0.9.x branch (further I expect that a 0.9.x branch will die soon).

I'm new to pre-commit too. I don't have a full understanding of what's possible yet.

This is the second point that makes me ponder: in the dev-lint branch we now have 3 different test & lint approaches / tools

1. we have the unit test in the github CI (as shown in the code snippet above)
2. we have pylint and other lint tools in the pyproject.toml (will be installed in a virtualenv of the developer).
3. additional we will have similar tools installed by: .. and pre-commit manages the installation and execution of any hook written in any language before every commit.

---

The decision for or against pre-commit hooks should not be made before the 0.9.3 branch point has been created. In py2.7 this tool will probably not work.

Furthermore, the decision should be carefully considered. The method of hooking the tests into the (git) pre-commit hooks is discussed controversial: https://www.reddit.com/r/git/comments/16ke0xa/arguments_for_and_against_precommit_hooks/

[da4089]

The tools don't run in Python 2.7. I tried in a venv without success.

[rsb-23]

@return42 Thanks for all the details, I would have shared the same.

• Most of the tools like pre-commit, pyproject, etc. are not compatible with Python 2.7
• Pre-commit purpose is to check basic syntax and rules before any commit (can be changed to pre-push action as well, but lets don't) which prevent syntactical error in repo.
• It appears slow as it runs multiple hooks, but its same per hook.
• Although unit test can be added to pre-commit, but it would require to rewrite tests in pytest or manually configure unittest. Also it won't be as effective and easy as in CI triggered on PR.
• Unittest / pytest along with coverage module are generally used in CI to generate test report and apply minimum coverage criteria.

My setup idea:

1. Adding all linters and formatter as part of pre-commit. (this installs all hooks locally)
2. pyproject.toml for the configurations like line-length (its not consistent as of now, I think 100 is good.), exclusions, etc.
3. Running linters using git add . && pre-commit run. Each hook can be run alone too.
4. unittest + coverage for unit tests and report generation.

[return42]

@da4089 About code-cleanup: the source files in this project are using CRLF (\r\n) to denote the newline character, while Unix-like systems represent it using LF (\n). Do you want to stay with CRLF or do you want to switch to LF?

Be warned: The switch to LF will touch ever single file / conflict with all forks and branches.

[da4089]

@da4089 About code-cleanup: the source files in this project are using CRLF (\r\n) to denote the newline character, while Unix-like systems represent it using LF (\n). Do you want to stay with CRLF or do you want to switch to LF?

We should switch the repo to LF, and those working on Windows should use the automagic LF/CRLF translation built into git.

Be warned: The switch to LF will touch ever single file / conflict with all forks and branches.

Perhaps you could create a GitHub issue for this, and we'll pick a good moment to do it when all branches are merged?

[da4089]

One other approach might be to create the 0.9.x branch soon (more or less now).

The changes resulting from eg. black, and fixes to lint-identified issues should then be applied first to the 0.9.x branch, and then merged to master.

We could leave 0.9.x branch using setup.py, without pre-commit, and maybe just running the unit tests. master could get the pyproject.toml migration, pre-commit, and all the automation.

It's more work to merge all the formatting, etc, changes through both branches, but the tooling setup might be better this way?

[return42]

We should switch the repo to LF,

OK ..

One other approach

give me a little more time / I'm currently working on a commit line (thats why I ask al thes questions :) .. hopefully tomorrow I can present you a way that fulfill all your request :)

---

Be warned: The switch to LF will touch ever single file / conflict with all forks and branches.

I have to correct .. only some files have CRLF ..

[rsb-23]

Be warned: The switch to LF will touch ever single file / conflict with all forks and branches.

Perhaps you could create a GitHub issue for this, and we'll pick a good moment to do it when all branches are merged?

We can add this hook. And can also add this to CI for PR too.

  - repo: https://github.com/Lucas-C/pre-commit-hooks
    rev: v1.5.5
    hooks:
      - id: remove-crlf

[return42]

hopefully tomorrow I can present you a way that fulfill all your request :)

it's more than I expected / I'm still working on it ... there's a preview available in a PR I created in my fork for testing purposes.