Added linters and pre-commit hooks #36

rsb-23 · 2024-03-27T10:06:58Z

⚠️ Python compatibility 3.7 and above only.

Checks :

42 testcases ran successfully. (skipped=1)
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 · 2024-03-27T11:43:22Z

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 ?

da4089 · 2024-03-27T11:45:02Z

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 · 2024-03-27T11:46:56Z

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 · 2024-03-27T11:55:26Z

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.

da4089 · 2024-03-27T12:26:49Z

vobject/base.py

@@ -689,13 +700,13 @@ def __str__(self):
        if self.name:
            return "<{0}| {1}>".format(self.name, self.getSortedChildren())
        else:
-            return u'<*unnamed*| {0}>'.format(self.getSortedChildren())
+            return "<*unnamed*| {0}>".format(self.getSortedChildren())


Python 2.x issue

black did this, will check

I think we'll need to use a Black version that supports a target of py27. Which I think means 21.12b0 (I've tried pinning that and it seems to work).

I haven't yet compared the output side-by-side with this PR.

da4089 · 2024-03-27T12:31:35Z

vobject/hcalendar.py

@@ -12,7 +12,7 @@
 DTSTART:20051005
 DTEND:20051008
 SUMMARY:Web 2.0 Conference
-LOCATION:Argent Hotel\, San Francisco\, CA
+LOCATION:Argent Hotel, San Francisco, CA


I think the correct fix here is to use a raw string, and preserve the "\," which are correct iCalendar syntax?

da4089 · 2024-03-27T12:33:00Z

vobject/hcalendar.py


            # DTSTART
            dtstart = event.getChildValue("dtstart")
            if dtstart:
-                if type(dtstart) == date:
+                if type(dtstart) is date:


I replaced these with

isinstance(dtstart, date)

which I like better, because it allows the use of a derived type if you want/need to do that for some reason.

class datetime(date)

Due to this reason, type is a simpler way as it doesn't depend on the order of if/else. Using isinstance will require reordering the code in all conditions and make sure it stays likewise in future. (maintainability issue)

Ideally, there shouldn't be so many type/instance checks. So, I kept the logic as it is (to refactor later) and cleared linting error only.

return42 · 2024-03-27T12:35:00Z

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 · 2024-03-27T13:19:34Z

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 · 2024-03-27T13:43:03Z

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 · 2024-03-27T13:46:11Z

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 · 2024-03-27T22:18:30Z

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 · 2024-03-28T18:40:29Z

@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:

vobject/.github/workflows/test.yml

Line 8 in 8f72c86

python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12']

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

return42 · 2024-03-28T19:29:40Z

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 · 2024-03-28T22:03:49Z

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 · 2024-03-28T22:05:16Z

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

return42 · 2024-03-29T07:09:40Z

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 ...

vobject/.github/workflows/test.yml

Line 8 in 8f72c86

python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12']

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):

we do not know what lint tools are work in py2.7 (do we need the tool itself in the branch?)
IDK if pyproject.toml is supported well by python's pip & setup tools in Py 2.7 (I have my doubts)
I expect that pre-commit will not work in python 2.7
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

we have the unit test in the github CI (as shown in the code snippet above)
we have pylint and other lint tools in the pyproject.toml (will be installed in a virtualenv of the developer).
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 · 2024-03-29T10:19:14Z

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

rsb-23 · 2024-03-29T10:45:07Z

@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:

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

return42 · 2024-03-29T12:24:23Z

@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 · 2024-03-29T12:45:16Z

@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 · 2024-03-29T12:50:16Z

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 · 2024-03-29T13:03:21Z

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 · 2024-03-29T14:40:05Z

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 · 2024-03-30T10:39:48Z

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.

- lint changes by tools

da4089 · 2024-12-19T11:37:53Z

@rsb-23, can we revisit this now that we have separate 0.9 and 1.0 branches?

I think we're good now to add to pyproject.toml and work towards enabling a pre-commit hook?

Linter fixes

2deaa94

da4089 reviewed Mar 27, 2024

View reviewed changes

da4089 mentioned this pull request Mar 27, 2024

Linter fixes integration #37

Merged

All configs moved to toml

2630c9f

- lint changes by tools

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added linters and pre-commit hooks #36

Added linters and pre-commit hooks #36

rsb-23 commented Mar 27, 2024

return42 commented Mar 27, 2024

da4089 commented Mar 27, 2024

da4089 commented Mar 27, 2024

da4089 commented Mar 27, 2024

da4089 Mar 27, 2024

rsb-23 Mar 27, 2024

da4089 Mar 27, 2024

da4089 Mar 27, 2024 •

edited

Loading

da4089 Mar 27, 2024 •

edited

Loading

rsb-23 Mar 27, 2024

return42 commented Mar 27, 2024

da4089 commented Mar 27, 2024 •

edited

Loading

rsb-23 commented Mar 27, 2024

rsb-23 commented Mar 27, 2024

da4089 commented Mar 27, 2024

return42 commented Mar 28, 2024

return42 commented Mar 28, 2024

da4089 commented Mar 28, 2024

da4089 commented Mar 28, 2024

return42 commented Mar 29, 2024

da4089 commented Mar 29, 2024

rsb-23 commented Mar 29, 2024

return42 commented Mar 29, 2024

da4089 commented Mar 29, 2024

da4089 commented Mar 29, 2024

return42 commented Mar 29, 2024 •

edited

Loading

rsb-23 commented Mar 29, 2024

return42 commented Mar 30, 2024

da4089 commented Dec 19, 2024

Added linters and pre-commit hooks #36

Are you sure you want to change the base?

Added linters and pre-commit hooks #36

Conversation

rsb-23 commented Mar 27, 2024

return42 commented Mar 27, 2024

da4089 commented Mar 27, 2024

da4089 commented Mar 27, 2024

da4089 commented Mar 27, 2024

da4089 Mar 27, 2024

Choose a reason for hiding this comment

rsb-23 Mar 27, 2024

Choose a reason for hiding this comment

da4089 Mar 27, 2024

Choose a reason for hiding this comment

da4089 Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

da4089 Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

rsb-23 Mar 27, 2024

Choose a reason for hiding this comment

return42 commented Mar 27, 2024

da4089 commented Mar 27, 2024 • edited Loading

rsb-23 commented Mar 27, 2024

rsb-23 commented Mar 27, 2024

da4089 commented Mar 27, 2024

return42 commented Mar 28, 2024

return42 commented Mar 28, 2024

da4089 commented Mar 28, 2024

da4089 commented Mar 28, 2024

return42 commented Mar 29, 2024

da4089 commented Mar 29, 2024

rsb-23 commented Mar 29, 2024

return42 commented Mar 29, 2024

da4089 commented Mar 29, 2024

da4089 commented Mar 29, 2024

return42 commented Mar 29, 2024 • edited Loading

rsb-23 commented Mar 29, 2024

return42 commented Mar 30, 2024

da4089 commented Dec 19, 2024

da4089 Mar 27, 2024 •

edited

Loading

da4089 Mar 27, 2024 •

edited

Loading

da4089 commented Mar 27, 2024 •

edited

Loading

return42 commented Mar 29, 2024 •

edited

Loading