search

pe-st / garmin-connect-export

Download a copy of your Garmin Connect data, including stats and GPX tracks.
MIT License
374 stars 77 forks source link

Remove Python2 leftovers #73

Closed bxsx closed 2 years ago

bxsx commented 2 years ago

Story

This PR removes all traces of Python2 support

Background

Python2 is not supported since v3.2.0 (via 525e3fd). Rationales: #64

Details

Note:

Python 2 is still referenced in the documentation. These references are intended to be kept for some time. https://github.com/pe-st/garmin-connect-export/blob/0b114e722b73da04e905a5c50caf4b8b7edeaff8/CONTRIBUTING.md?plain=1#L11-L16

pe-st commented 2 years ago

Thank you for your PR!

I see you also cleaned up some code that it not related to Python 3, but to pylint. I didn't keep the code pylint-clean during the transition period Python 2..3, so there is no automated linting yet.

The documentation for GitHub Actions for Python mentions flake8 (but not pylint), so flake8 is probably the preferred/a good enough tool to keep the style clean. I would like to have some tool in Github Actions to help people like me that don't use python daily to point at things that could be done better. And with removing python2 code it becomes once more feasible to use such a tool.

I have added a feature branch with a draft on how flake8 could be integrated.

What do you think about it, @bxsx ?

bxsx commented 2 years ago

Honestly, I didn't run any code checkers on this, so all changes were done manually but I do have some linters installed in my head anyway ;-)

To answer your question. I would recommend using black - an official tool curated by Python Software Foundation. This is quite similar to gofmt in Go and keeps the whole codebase within the recommended style without any additional questions or comments from anyone. Simply set it and forget it (and let PSF to set the standard for your code).

Here are GitHub Actions docs: https://black.readthedocs.io/en/stable/integrations/github_actions.html

I created a gist to demonstrate diff output when black is run with different options for easier reviewing: https://gist.github.com/bxsx/29051fd063f4c92c249338fcba06e66a

There are 3 diff files:

Take a look on the diffs output. I can open a PR with the black integration and required changes, and maybe manually review the style, so I can fix or compact something.

pe-st commented 2 years ago

Take a look on the diffs output. I can open a PR with the black integration and required changes, and maybe manually review the style, so I can fix or compact something.

From what I saw black is just a formatter, not a static analyzer. I think it is maybe a little bit too opinionated (in some cases where I added newlines for readability, black removed them etc)

I hope I'll have some more time in the following weeks to look into the available code checkers, but I'll merge your PR before

bxsx commented 2 years ago

From what I saw black is just a formatter, not a static analyzer. I think it is maybe a little bit too opinionated (in some cases where I added newlines for readability, black removed them etc)

That's right. It's a formatter that complies with the official Python formatting standards. Just as indentation is very strict in Python, newlines or doubled-newlines have a common meaning, and this is the reason black removed those lines. They were against the rules of PEP-8. Of course, you may not adhere to the standard.

Anyway, the main feature of Flake8 is code checking for PEP-8 (the reason why 8 is in the name), which is why I referred to black.

I hope I'll have some more time in the following weeks to look into the available code checkers

In that case, the recommended tool is mypy, but I'm afraid the code base is not ready for this.

I recommend using both Flake8 and black in the pipeline, and we can start supporting Mypy from there.

but I'll merge your PR before

Thanks!

PS. There are nice SaaS solutions that are free for Open Source projects, such as https://codeclimate.com or https://sonarcloud.io

bxsx commented 2 years ago

In that case, the recommended tool is mypy, but I'm afraid the code base is not ready for this.

So I ran mypy and fixed the reported issues in 30b6b0b. It runs successfully now, but without supported typing we don't take the real advantage of mypy. See output when run with --strict option: Found 334 errors in 4 files (checked 4 source files)

pe-st commented 2 years ago

I rebased and merged the PR. Thanks again!

bxsx commented 2 years ago

Thanks @pe-st!

One question, why does GitHub show the PR as rejected? I see commits in the master branch though..

pe-st commented 2 years ago

Thanks @pe-st!

One question, why does GitHub show the PR as rejected? I see commits in the master branch though..

I rebased your commits in my local workspace, not via the Github interface, and closed the PR manually. Maybe I missed some option/button to declare the PR as merged. I always struggle with the Github interface, at work I'm used to Gitlab...

bxsx commented 2 years ago

Gotcha! Well, seems like GitHub doesn't have a way to know if rebased commits are attached to any PR. I think that's why GH allows to rebase and merge via UI interface. Some docs on the topic: Supported merge options by GitHub and Merge UI instructions

PS. GitLab is very nice too!