Python files style cleanup

[redphix]

What does this PR do?

Style clean up using black, isort and flake8

This PR touches only /api/*py files. Not including the sub directories (eg /api/lightning, etc). Will be appropriate to take in another PR.
I have isolated the changes logically into different commits to make it easier to review by commit.
@Reckless-Satoshi Let me know what you think. We can use this to discuss your gripes with black and try to configure it accordingly.

Here's a list of PEP8 linting "errors" that were fixed using flake8:

• Do not use bare except, specify exception instead (E722)
• Comparison to None should be 'cond is None:' (E711)
• Test for membership should be 'not in' (E713)
• Test for object identity should be 'is not' (E714)
• Comparison to true should be 'if cond is true:' or 'if cond:' (E712)

[Reckless-Satoshi]

Thanks a lot for checking over these basics. Is this PR ready for review?

We might also want to add a lint / format workflow to check over each of our PRs as a Github Action. I believe it can be done by editing https://github.com/Reckless-Satoshi/robosats/blob/main/.github/workflows/linter.yml . The wearerequired/lint-action@v2 action supports black and flake8 .

      - name: Run linters
        uses: wearerequired/lint-action@v2
        with:
          black: true
          black_dir: api
          flake8: true
          flak8_dir: api

[redphix]

@Reckless-Satoshi

Is this PR ready for review?

Yup, mostly. Just fixing the conflicts and adding the linting action should make it complete. But again, just touched the api/*.py files. You think I should make it complete with running and fixing all the *py files?

[Reckless-Satoshi]

@redphix

What if in this PR we only merge the changes to .flake, pyproject.toml, requirements.txt and add the GH workflow and instructions on how to run black in setup.py .

We could also edit .github/pull_request_template.md and add - [ ] If it's a backend feature, I have ran ...

Next to merging it, I open another PR where I run black for the first time over all of the *py . That will make reviewing easier (not needed), as otherwise it could be a hazard to merge a PR where some many lines change at once (can't really check over every single change).

[redphix]

@redphix

What if in this PR we only merge the changes to .flake, pyproject.toml, requirements.txt and add the GH workflow and instructions on how to run black in setup.py .

We could also edit .github/pull_request_template.md and add - [ ] If it's a backend feature, I have ran ...

Next to merging it, I open another PR where I run black for the first time over all of the *py . That will make reviewing easier (not needed), as otherwise it could be a hazard to merge a PR where some many lines change at once (can't really check over every single change).

Sounds good. Also I suggest using https://pre-commit.com/ for managing pre-commit git hooks and running things like pretier, flake, black and also maybe running the command to generate the oas schemas automatically as json/yaml and placing them where required, every time someone commits. But this requires the contributors to install and configure pre-commit python package on their systems.

Also I have heard real good things about deepsource.io. I wanted to give it a try. It can run those linters and also make commits to fix those linting errors automatically on each PR (will cover people who don't configure pre-commit hooks)

[redphix]

I'll try including a pre-commit config here as well if you agree on including it...

[Reckless-Satoshi]

Sounds good. Also I suggest using https://pre-commit.com/ for managing pre-commit git hooks

That sounds good!

Before going into deepsource.io for fixing PR lint errors, we should try to do it with our workflow .github/workflows/linter.yml . I believe it can also be easily configured to run the linters and fix errors on PRs in addition to simply checking.

[redphix]

I believe it can also be easily configured to run the linters and fix errors on PRs in addition to simply checking.

Yup it can with linters that support it. Flake8 for instance, doesn't support fixing the issues, but just points it out. There's autopep8 for that I guess. But I don't want to include that, since it's just too much (atleast for now). Another thing to note is that any linting solution (including deepsource) can't actually auto-fix on forked repos, but it should be able to report it? Not sure...will have to test it.

[redphix]

Okay, so turns out we can't use either auto-fix or commit annotations using linters. I was hoping the actions ran on each PR touching py files and atleast report the linting errors, so that it can be fixed, but even doing that is tricky.

From the readme of wearerequired/lint-action

Limitations

Pull requests

There are currently some limitations as to how this action (or any other action) can be used in the context of pull_request events from forks:

• The action doesn't have permission to push auto-fix changes to the fork. This is because the pull_request event runs on the upstream repo, where the github_token is lacking permissions for the fork. Source
• The action doesn't have permission to create annotations for commits on forks unless you use the pull_request_target event. You can modify the default permissions granted to the GITHUB_TOKEN by using the permissions key and set the checks scope to write. See GitHub documentation for more.

Even though pull_request_target fix is mentioned to atleast be able to have commit annotations, it doesn't seem to work for me. Maybe it doesn't work on a PR and has to be merged first? not sure, but all in all, it doesn't seem like we can have this for forked repos.

I am leaving this with the auto_fix option as true and the job will fail because it won't have permission to commit to my fork, but you can run it as you mention earlier by opening a PR yourself

[redphix]

Seems like incorporating pre-commit in our dev workflow would be the logical step to enforce the style guides and lint fixes. Will write one up tomorrow, need your inputs for ts/js files. We can still have the above workflows, but only for pushes to main or when you open a PR to main?

[Reckless-Satoshi]

Even though pull_request_target fix is mentioned to atleast be able to have commit annotations, it doesn't seem to work for me. Maybe it doesn't work on a PR and has to be merged first? not sure, but all in all, it doesn't seem like we can have this for forked repos.

Do we at least get commit annotations if we relax the GITHUB_TOKEN permissions?
E.g. add permissions key before the jobs and use the pull_request_target event.

permissions:
  checks: write

[Reckless-Satoshi]

I guess this action will always fail on PRs if auto_fix is true.

[redphix]

yup, I mentioned it above. It will fail because it won't be able to push commits to forks.

[redphix]

Even though pull_request_target fix is mentioned to atleast be able to have commit annotations, it doesn't seem to work for me. Maybe it doesn't work on a PR and has to be merged first? not sure, but all in all, it doesn't seem like we can have this for forked repos.

Do we at least get commit annotations if we relax the GITHUB_TOKEN permissions? E.g. add permissions key before the jobs and use the pull_request_target event.

permissions:
  checks: write

Tried that, didn't work. I am not sure why. Maybe you have to merge this PR first and then test it?